Skip to content

Add compose v5 compatibility, legacy-field warnings, UI error fixes - #2094

Open
SohamKukreti wants to merge 2 commits into
developfrom
fix/docker-deploy-0.9-gaps
Open

Add compose v5 compatibility, legacy-field warnings, UI error fixes#2094
SohamKukreti wants to merge 2 commits into
developfrom
fix/docker-deploy-0.9-gaps

Conversation

@SohamKukreti

Copy link
Copy Markdown
Collaborator

Summary

Companion to the self-hosting docs update; all changes verified against a built 0.9.2 image.

docker-compose.yml:

  • Express the PID cap as deploy.resources.limits.pids instead of pids_limit: Compose v5 normalizes the two together and rejected the file ("can't set distinct values on 'pids_limit' and 'deploy.resources.limits.pids'"), which also broke compose down. Identical behavior on Compose v2.x.

entrypoint.sh:

  • When no CRAWL4AI_API_TOKEN is set, explain that the loopback bind is the container's own (published ports reset connections) and how to fix it. The old one-line notice read as "available at 127.0.0.1:11235" and sent users debugging the wrong thing.

server.py / schemas.py:

  • /screenshot and /pdf: output_path (removed in 0.9.0) was silently dropped; requests still succeed but now return a warning that no file was written and results are fetched via GET /artifacts/{artifact_id}.
  • Legacy hooks.code (removed in 0.9.0) parsed as an empty hook list and reported {"status": "success", "attached": []} with hooks enabled. The field is now captured (never executed) and the response reports status "ignored" plus a warning; /crawl/stream sends X-Hooks-Warning. With hooks disabled, any hooks payload still returns 403 (unchanged).
  • Add "/" to the auth gate's exact-match public paths so the existing root -> /playground redirect is reachable; previously the middleware returned a bare 401 before the route ran. /monitor and all data routes remain gated.

static/playground/index.html:

  • The streaming branch never checked response.ok and showed a green "Success" banner for 401 bodies; it now errors before reading the stream. All branches surface the server's detail message instead of a generic "Request failed", and 401s hint at the token bar (distinguishing "no token set" from "token rejected").

tests:

  • Add deploy/docker/tests/test_legacy_compat.py covering the root redirect, output_path warning, legacy-hooks handling, and the compose PID-cap placement (13 behavioral tests, TestClient-based).

Fixes #2091

List of files changed and why

  • deploy/docker/entrypoint.sh
  • deploy/docker/schemas.py
  • docker/server.py
  • docker/static/playground/index.html
  • deploy/docker/tests/test_legacy_compat.py
  • docker-compose.yml

How Has This Been Tested?

Ran old tests and also created a new test file to test the changes. Manually verified as well.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added/updated unit tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

…und error handling

- docker-compose.yml: move the PID cap to deploy.resources.limits.pids
  (Compose v5 rejects pids_limit alongside a limits block; same behavior
  on v2.x).
- entrypoint.sh: explain the loopback-only bind when no CRAWL4AI_API_TOKEN
  is set and how to fix it.
- server.py: warn when the removed output_path (screenshot/pdf) or legacy
  hooks.code is sent - fields are ignored, never executed; hooks status
  reads "ignored" and /crawl/stream sends X-Hooks-Warning. Make "/"
  public so the /playground redirect works; data routes stay gated.
- schemas.py: capture legacy hooks.code so it can be reported (never run).
- playground: check response.ok on the streaming branch, surface server
  error details, hint at the token bar on 401.
- tests: add deploy/docker/tests/test_legacy_compat.py (13 tests).
@SohamKukreti
SohamKukreti force-pushed the fix/docker-deploy-0.9-gaps branch from 614593c to 12d138b Compare July 22, 2026 12:57
@SetagGnaw

SetagGnaw commented Jul 25, 2026

Copy link
Copy Markdown

Nice work on the legacy-compat warnings. One interaction seems worth a look.

The pre-existing gate in /crawl and /crawl/stream fires on the mere presence of a hooks object, without looking inside it:

if crawl_request.hooks and not HOOKS_ENABLED:
    raise HTTPException(403, "Hooks are disabled. Set CRAWL4AI_HOOKS_ENABLED=true to enable.")

crawl_request.hooks parses into a truthy HookConfig whether the payload carries declarative hooks, legacy code, or both, so all three shapes get the same 403 with the same advice. Before this PR the gate could not do better: pydantic dropped the unknown code field, so a code-only payload was indistinguishable from an empty one by the time it reached the gate. Your schema change is what makes the distinction expressible at all (hooks.code and hooks.hooks both survive parsing now), but the PR only uses it after the crawl, in the warning block.

Since CRAWL4AI_HOOKS_ENABLED defaults to false, that ordering means a legacy 0.8.x caller on a stock deployment never reaches the new hooks.status: "ignored" + warning path. They hit the 403 first, and its remedy is misleading for them twice over: enabling the flag won't make inline code run (it was removed entirely in 0.9.0), and it nudges operators into enabling the declarative-hooks surface just to appease code that can never execute. I see test_any_hooks_payload_403_when_disabled pins the 403-when-disabled behavior, so I take the gate itself as deliberate. The ask is only that the gate use the distinction your schema change just made possible.

Smallest version, no status codes change: keep firing on presence, but branch the error detail on what is actually inside, e.g.:

if crawl_request.hooks and not HOOKS_ENABLED:
    if crawl_request.hooks.code:
        raise HTTPException(
            403,
            "Inline hook code (hooks.code) was removed in 0.9.0 and cannot be "
            "enabled; it was not executed. Use declarative hook actions instead "
            "(GET /hooks/info), which are additionally disabled on this server "
            "(CRAWL4AI_HOOKS_ENABLED).",
        )
    raise HTTPException(403, "Hooks are disabled. Set CRAWL4AI_HOOKS_ENABLED=true to enable.")

Probably worth hoisting into a small helper, since the gate appears in both /crawl and /crawl/stream.

Related question: did you consider rejecting hooks.code payloads with a 400 in both cases (hooks enabled and hooks disabled), with a clear "removed in 0.9.0" error, instead of 200 + warning? A caller that never inspects the warning field reads the 200 as "my hook ran", which is the same silent-failure shape this PR is eliminating, just moved from the field level to the status level. Curious whether backward compatibility drove the accept-and-warn choice.

Per review: enabling CRAWL4AI_HOOKS_ENABLED cannot run inline hook code
(removed in 0.9.0), so code-carrying payloads now get a removal message
instead of the misleading generic hint.
@SohamKukreti

SohamKukreti commented Jul 25, 2026

Copy link
Copy Markdown
Collaborator Author

@SetagGnaw Good catch on the ordering. On a stock deployment (CRAWL4AI_HOOKS_ENABLED defaults to false) a legacy hooks.code caller hits the presence-based 403 and never reaches the "ignored" + warning path, and the 403's remedy is actively misleading for them: enabling the flag won't ever make inline code run.

I've adopted your suggested shape: keep firing on presence (the 403-when-disabled contract stays pinned by test_any_hooks_payload_403_when_disabled), branch only the detail on hooks.code, and hoist it into a small helper since the gate appears in both /crawl and /crawl/stream plus a test pinning the code-only 403 message so the two error texts don't drift back together.

On your 400 vs. 200 + warning question: I considered rejecting hooks.code payload with a 400, and backward compatibility did drive it, but with a specific shape in mind:

  1. A 400 now would be a second breaking change for the same clients. Since 0.9.0 (two releases), a hooks.code payload with hooks enabled has returned 200 with complete, usable crawl results, the inline code never influenced the crawl in 0.9.x, so the results aren't degraded by its absence, only the hook side-effects are missing. Pipelines that adapted by simply not caring about hook effects (or that only ever used hooks for nice-to-haves like scroll/settle) currently work; a 400 would break them to protect callers who depend on hook effects, and those callers already see broken output, so they have a detection signal today.
  2. Rejecting discards work the server can fully perform. The request minus code is a completely valid 0.9 crawl. That's also why mixed payloads argue against 400: declarative hooks + legacy code would be rejected wholesale even though everything except the dead field executes correctly.
  3. It's consistent with the output_path decision in the same PR: serve the thing we can serve (crawl results / artifact), attach an explicit machine-readable signal about the part we can't (warning, status: "ignored", X-Hooks-Warning on the stream). One philosophy across both legacy fields.

Thanks for the review!

@ntohidi

ntohidi commented Jul 27, 2026

Copy link
Copy Markdown
Collaborator

Nice work Soham — the #2091 diagnosis was thorough and these fixes are well-targeted. A few things to sort out before merge.

Blocking

1. The compose change silently disabled an existing security test.
deploy/docker/tests/test_security_container_posture.py:106 is assert "pids_limit" in compose against raw file text. You removed the key, but your new comment contains the words "not as pids_limit" — so the test still passes on a comment and no longer checks anything. Rewrite it to parse the YAML and assert deploy.resources.limits.pids == 512, or delete it in favour of your TestComposeFile test which already does this properly.

Also, compose config proves the key parses, not that it's applied. Please confirm once with docker inspect --format '{{.HostConfig.PidsLimit}}' <container> on a running stack — if that comes back 0, the PID cap is gone.

2. Issue item 2 is unaddressed, and it undercuts your own item-3 fix.
docker-compose.yml:7-8 still has env_file: - .llm.env with no required: false, and the environment: block (lines 9-19) is still commented out. A fresh clone still fails with "env file .llm.env not found", and the new entrypoint message tells users to set CRAWL4AI_API_TOKEN — which compose has no way to forward. Please add required: false, a CRAWL4AI_API_TOKEN=${CRAWL4AI_API_TOKEN:-} passthrough, and a commented token line in .llm.env.example.

While you're in that file: version: '3.8' is still line 1 (the issue flagged it — warns on every compose command).

3. pyyaml is missing from deploy/docker/tests/requirements.txt.
The new test file imports yaml; it only passes today via a transitive dep.

4. HookConfig.code: Optional[Dict[str, str]] is too strict.
MIGRATION.md only says "Python strings" — the exact 0.8.x shape was never pinned. A legacy caller whose payload doesn't match that type now gets a 422 on a field we document as ignored, which is worse than the silent drop it replaces. Use Optional[Any] so everything parses and reaches the removal message.

Should fix

output_path detection. (await request.json()).get("output_path") works, but it's duplicated across two handlers, invisible in OpenAPI, and sits inside the broad except Exception — so a surprise there becomes a 500 instead of a warning. It also only fires on the success path, so a legacy caller whose crawl fails never learns the field was dropped. Declaring it as a deprecated schema field on ScreenshotRequest/PDFRequest solves all three and matches what you did for hooks.code.

Hooks response shape. results.setdefault("hooks", {"attached": []}) — if the handler returns no hooks key and the request also carried declarative specs, the client gets a hooks object with no status. Seed it as {"status": "ignored", "attached": []}.

/monitor test. test_monitor_and_data_routes_stay_gated asserts /monitor → 401. I know the intent is proving the / change didn't widen the gate, but as written, adding the /monitor → /dashboard redirect the issue asks for would break this test. Assert "401 without a token" rather than "no redirect exists".

Playground. Two follow-ons: FastAPI 422s return detail as an array, so it renders as [object Object] — coerce non-strings. And only the streaming branch got the .catch(() => ({})) guard for non-JSON error bodies; the LLM and regular branches still call await response.json() bare, so a proxy HTML 502 throws a parse error there.

Entrypoint message. The bind guard at entrypoint.sh:27 accepts CRAWL4AI_API_TOKEN or CRAWL4AI_JWT_ENABLED=true, but the remedy line names only the token. Mention both.

Needs a call from @unclecode

Issue item 4 says "reject (or warn on)" output_path, so your warning satisfies it. Item 5 asks for an explicit 400 in both states for hooks.code, and this returns 200-with-warning when hooks are enabled. A warning key in a 200 body is still something an upgrading script won't read. I'd suggest warn this release, reject next, with the window written into MIGRATION.md — but let's decide deliberately.

Deferred / nits

  • Issue item 3's healthcheck question (should the container report healthy while the published port is dead?) is untouched. Fine to defer — just note it in the PR body so it isn't lost.
  • X-Hooks-Warning is header-only, and /crawl with stream: true diverts into stream_process before the body-level warning, so that path gets the header and nothing else. Worth documenting at minimum.
  • Whitespace-only churn in server.py (~lines 892, 900, 966) adds diff noise.

If you'd rather keep this PR tight, narrow the "Fixes #2091" claim and open a follow-up for the rest — that works for me too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants