From 083944b0c0dc9492e3bdd2e184c5ff65d1947abc Mon Sep 17 00:00:00 2001 From: Yuanwen Tian Date: Thu, 6 Aug 2026 11:51:34 +0800 Subject: [PATCH 1/3] fix: auto-download URL-sourced documents on view/crop with notice and progress MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A plain `ade view ` on a URL-sourced parse reported pages_embedded: 0 unless the user knew to add --download — an empty preview was a worse surprise than an implicit fetch (QA follow-up on the v1.0.2 doc-preview ticket). `view` and `crop` now detect a URL-sourced item with no attached copy and fetch it by default: a stderr notice says why the network is being touched and how to skip it (--no-download), and the download streams behind the guarantee progress line (percent + elapsed on a tty, decade lines piped, silent under --json). The fetch is never load-bearing unless asked: in auto mode an expired pre-signed URL degrades to the existing empty-preview note (payload records downloaded: false + download_error) so yesterday's working view never starts exiting non-zero; explicit --download keeps the strict erroring contract. crop hard-fails on a failed fetch as the no-copy state always did — a crop has no empty-imagery fallback. attach.py's consent contract is restated: a URL fetch is either asked for at parse time (--keep-copy) or said out loud before it starts. Co-Authored-By: Claude Fable 5 --- README.md | 16 +++--- docs/reference/help.json | 21 ++++++-- src/ade_cli/attach.py | 84 +++++++++++++++++++++++++----- src/ade_cli/crop.py | 69 +++++++++++++++++++++++-- src/ade_cli/help.py | 13 +++-- src/ade_cli/parse.py | 10 ++-- src/ade_cli/view.py | 108 +++++++++++++++++++++++++-------------- tests/test_crop.py | 70 +++++++++++++++++++++++-- tests/test_serve.py | 14 ++--- tests/test_view.py | 85 +++++++++++++++++++++++++++--- 10 files changed, 395 insertions(+), 95 deletions(-) diff --git a/README.md b/README.md index d638cde..d9af75c 100644 --- a/README.md +++ b/README.md @@ -122,7 +122,7 @@ open it exits immediately, naming all three paths. |---|---| | `ade parse -d invoice.pdf` | ensure parsed; artifacts land in `~/.ade` | | `ade parse --document-url https://…/doc.pdf` | server fetches the URL | -| `ade parse --document-url https://… --keep-copy` | also store the document bytes locally, so page previews and crops render | +| `ade parse --document-url https://… --keep-copy` | also store the document bytes locally, while the (often pre-signed) URL still works | | `ade parse -d doc.pdf --tier standard --wait 0` | cheap lane, submit-and-return | | `ade parse -d doc.pdf --env eu` | run in the EU region (or `export ADE_ENV=eu`) | | `ade parse -d doc.pdf --include markdown --json` | carry the markdown in the payload | @@ -203,12 +203,14 @@ On a terminal, `view` opens the viewer in your browser by default (`--no-open` suppresses it); `--json` runs and piped output never launch a browser — the artifact path is in the output either way. Without a JOB_ITEM_ID, `view` targets the latest viewable job item. -URL-parsed items have no local document bytes, so page previews can't -render until you fetch them: `ade view --download` attaches a copy -of the document to the job item (plain HTTP, no API credits — but note -pre-signed URLs expire, so `parse --keep-copy` at parse time is the -reliable way), after which previews and crops render from it with an -"unverified against the parsed run" caveat. +URL-parsed items have no local document bytes, so on first `view` or +`crop` the CLI fetches a copy of the document into the job item +automatically — announced on stderr with a progress line, plain HTTP, +no API credits (`--no-download` skips the fetch; previews then stay +empty). Note pre-signed URLs expire, so `parse --keep-copy` at parse +time is the reliable way to secure the bytes. Previews and crops render +from the attached copy with an "unverified against the parsed run" +caveat. Every job item gets a **self-contained `view.html`**: page images with bounding-box overlays beside the parsed markdown (or extraction JSON), diff --git a/docs/reference/help.json b/docs/reference/help.json index 4e8bf2e..d058fb7 100644 --- a/docs/reference/help.json +++ b/docs/reference/help.json @@ -472,7 +472,7 @@ "metavar": null, "required": false, "default": null, - "help": "--document-url only: also download the document into the job item (plain HTTP, no API credits) so page previews and crops render locally \u2014 fetched now, while the URL (often pre-signed) still works. Without it, URL parses have no local bytes and the viewer explains how to fetch them later (`ade view --download`)." + "help": "--document-url only: also download the document into the job item (plain HTTP, no API credits) so page previews and crops render locally \u2014 fetched now, while the URL (often pre-signed) still works. Without it, the first `view`/`crop` fetches the copy instead, by which time a pre-signed URL may have expired." }, { "flags": "--include", @@ -999,11 +999,11 @@ "help": "Pages to embed images for, 1-indexed, e.g. '1,3-5'." }, { - "flags": "--download", + "flags": "--download, --no-download", "metavar": null, "required": false, "default": null, - "help": "URL-parsed items: fetch the document from its recorded URL into the job item and render page previews from that copy \u2014 the parse itself never gives the CLI the bytes (#169). Plain HTTP, no API credits; the copy is unverified against the parsed run. Also works on an extract item id (fetches into its referenced parse item)." + "help": "URL-parsed items: fetch the document from its recorded URL into the job item and render page previews from that copy \u2014 the parse itself never gives the CLI the bytes (#169). This happens automatically when no copy is attached yet (a notice and progress line land on stderr); --no-download skips the fetch and previews stay empty. Explicit --download makes a failed fetch an error instead of a warning. Plain HTTP, no API credits; the copy is unverified against the parsed run. Also works on an extract item id (fetches into its referenced parse item)." }, { "flags": "--no-sidebar-sync", @@ -1061,7 +1061,11 @@ }, { "key": "downloaded", - "what": "with --download: true when this run fetched the URL document into the job item (false: already attached)" + "what": "URL items: true when this run fetched the document into the job item (automatic on first view; false: already attached, or the automatic fetch failed \u2014 see download_error)" + }, + { + "key": "download_error", + "what": "why the automatic fetch failed, when it did (the viewer still builds, previews empty; else absent)" }, { "key": "deep_link", @@ -1139,6 +1143,13 @@ "required": false, "default": null, "help": "Open the crop (or the directory holding them)." + }, + { + "flags": "--download, --no-download", + "metavar": null, + "required": false, + "default": null, + "help": "URL-parsed items: fetch the document from its recorded URL into the job item and crop from that copy \u2014 the parse itself never gives the CLI the bytes (#169). This happens automatically when no copy is attached yet (a notice and progress line land on stderr); --no-download skips the fetch, and the crop then fails honestly (a crop has no empty-imagery fallback)." } ], "supports_json": true, @@ -1331,7 +1342,7 @@ }, { "path": " document.", - "what": "URL parses: the attached document copy (`parse --keep-copy` / `view --download`) page previews and crops render from \u2014 unverified against the parsed run" + "what": "URL parses: the attached document copy (`parse --keep-copy`, or fetched automatically on first `view`/`crop`) page previews and crops render from \u2014 unverified against the parsed run" }, { "path": " view.html / crops/", diff --git a/src/ade_cli/attach.py b/src/ade_cli/attach.py index 188271a..1b2d23d 100644 --- a/src/ade_cli/attach.py +++ b/src/ade_cli/attach.py @@ -2,19 +2,22 @@ ``parse --document-url`` never hands the CLI the document bytes — the server fetches the URL — so page previews and crops have nothing local -to render from. An *attached copy* closes that gap on explicit consent: -``parse --keep-copy`` downloads the document at parse time (while the -URL — often pre-signed — still works), and ``view --download`` fetches -it after the fact. The copy lives inside the job item -(``jobs//document.``) and is recorded on meta.json as auxiliary -metadata: the recorded ``source`` stays the URL (provenance truth) and -the item id never moves. The raster layer falls back to the copy via -``renderable_source``. - -The CLI still never fetches a URL without one of these explicit flags — -and because it never saw the original bytes, an attached copy is not -verifiable against the parsed generation: renders from it carry the -``caveat`` note rather than posing as ground truth. +to render from. An *attached copy* closes that gap: ``parse +--keep-copy`` downloads the document at parse time (while the URL — +often pre-signed — still works), and ``view`` / ``crop`` fetch it on +first use — automatically when the item is URL-sourced and no copy is +attached yet, announced by a stderr notice + progress line +(``download_with_notice``), suppressible with ``--no-download``. The +copy lives inside the job item (``jobs//document.``) and is +recorded on meta.json as auxiliary metadata: the recorded ``source`` +stays the URL (provenance truth) and the item id never moves. The +raster layer falls back to the copy via ``renderable_source``. + +A URL fetch is therefore never *silent* — it is either asked for at +parse time or said out loud before it starts — and because the CLI +never saw the original bytes, an attached copy is not verifiable +against the parsed generation: renders from it carry the ``caveat`` +note rather than posing as ground truth. """ from __future__ import annotations @@ -25,6 +28,7 @@ from urllib.parse import urlparse import httpx +import typer from .store import JobStore, replace_with_retry @@ -118,13 +122,19 @@ def download( *, transport: httpx.BaseTransport, now: float, + progress=None, ) -> tuple[str, int]: """Fetch the item's URL source and attach the copy; returns ``(filename, bytes)``. Raises AttachError with the remediation — pre-signed URLs expire, so a late fetch failing is the expected - failure mode, not a surprise.""" + failure mode, not a surprise. ``progress`` (a ``guarantee.Progress``) + reports received/total per chunk; a body without Content-Length + still shows the moving label, just without a percentage.""" url = meta.get("source") or "" name = copy_name(url) + label = f"downloading {name}" + if progress is not None: + progress.update(label=label) target = jobs.item_dir(item_id) / name target.parent.mkdir(parents=True, exist_ok=True) tmp = target.with_name(f".{name}.tmp-{os.getpid()}") @@ -156,6 +166,7 @@ def download( f"{MAX_COPY_BYTES}-byte attach cap; parse the local " "file instead (ade parse -d ).", ) + total = int(declared) if declared.isdigit() else 0 with tmp.open("wb") as sink: for chunk in response.iter_bytes(): received += len(chunk) @@ -168,6 +179,10 @@ def download( ) digest.update(chunk) sink.write(chunk) + if progress is not None and total: + progress.update( + label=label, fraction=received / total + ) except httpx.HTTPError as error: tmp.unlink(missing_ok=True) raise AttachError( @@ -198,3 +213,44 @@ def download( ) jobs.write_json(item_id, "meta.json", current) return name, received + + +def download_with_notice( + jobs: JobStore, + item_id: str, + meta: dict, + *, + ports, + as_json: bool, +) -> tuple[str, int]: + """``download`` with the say-it-out-loud surface around it: a stderr + notice naming why the network is about to be touched and how to + suppress it, then a progress line while the body streams. stderr + only, fully silent under ``--json`` (the payload carries the + receipt); raises AttachError exactly like ``download``.""" + # Lazy import mirrors update.py's: guarantee.py imports update, which + # would make a top-level import here a needlessly heavy chain. + from .guarantee import Progress + + if not as_json: + typer.echo( + f"note: job item {item_id} was parsed from a URL and no local " + "copy is attached — downloading it so page imagery can render " + "(skip with --no-download)", + err=True, + ) + progress = Progress( + ports.clock, + "off" if as_json else ("tty" if ports.stderr_is_tty() else "plain"), + ) + try: + return download( + jobs, + item_id, + meta, + transport=ports.transport, + now=ports.clock.now(), + progress=progress, + ) + finally: + progress.close() diff --git a/src/ade_cli/crop.py b/src/ade_cli/crop.py index 434bb92..192982d 100644 --- a/src/ade_cli/crop.py +++ b/src/ade_cli/crop.py @@ -161,6 +161,7 @@ def find_element_or_exit( def crop( + ctx: typer.Context, job_id_token: str | None = typer.Argument( None, metavar="[JOB_ITEM_ID]", help="Job item id or unambiguous prefix." ), @@ -194,6 +195,16 @@ def crop( open_image: bool = typer.Option( False, "--open", help="Open the crop (or the directory holding them)." ), + download: bool | None = typer.Option( + None, "--download/--no-download", + help="URL-parsed items: fetch the document from its recorded URL " + "into the job item and crop from that copy — the parse itself " + "never gives the CLI the bytes (#169). This happens " + "automatically when no copy is attached yet (a notice and " + "progress line land on stderr); --no-download skips the fetch, " + "and the crop then fails honestly (a crop has no empty-imagery " + "fallback).", + ), as_json: bool = JSON_FLAG, ) -> None: """Crop element regions from the source document into PNGs: one @@ -234,11 +245,61 @@ def crop( selected = elements.select( records, element_type=element_type, page=page, element_ids=element_ids ) + parse_meta = jobs.read_json(parse_item_id, "meta.json") + if download and not attach.is_url_source(parse_meta): + message = ( + f"Job item {item_id} has no URL source to download: " + "--download applies to items parsed from --document-url " + "(local parses crop from their file directly)." + ) + exit_with( + { + "error": "not_a_url_source", + "job_item_id": item_id, + "message": message, + }, + message, + as_json=as_json, + code=EXIT_USAGE, + ) + # A URL parse without an attached copy has nothing local to crop + # from, so the copy fetches now by default — announced on stderr, + # suppressible with --no-download (#169 follow-up, mirroring `view`). + # Unlike view there is no degraded render to fall back to (a crop is + # never served from missing imagery), so a failed fetch is the + # command's failure, exactly as the no-copy state already was. + downloaded = None + if ( + download is not False + and selected + and attach.is_url_source(parse_meta) + and attach.attached_file(jobs, parse_item_id, parse_meta) is None + ): + try: + attach.download_with_notice( + jobs, + parse_item_id, + parse_meta or {}, + ports=ctx.obj, + as_json=as_json, + ) + except attach.AttachError as error: + exit_with( + { + "error": error.kind, + "job_item_id": parse_item_id, + "message": error.message, + }, + error.message, + as_json=as_json, + code=EXIT_FAILED, + ) + downloaded = True + parse_meta = jobs.read_json(parse_item_id, "meta.json") # One drift check per invocation, not per element: the batch renders # from a single recorded source, and hashing it once is the whole cost. # URL items have no drift check (no recorded content hash); a render # from their attached copy carries the unverified-bytes caveat instead. - parse_meta = jobs.read_json(parse_item_id, "meta.json") drift = source_drift_note(parse_meta) or attach.caveat( jobs, parse_item_id, parse_meta ) @@ -271,8 +332,9 @@ def crop( message = error.message if "parsed from a URL" in message: message += ( - f" Fetch it with `ade view {parse_item_id} --download`, " - "then re-run this crop." + " Re-run without --no-download to fetch the document, " + f"or fetch it with `ade view {parse_item_id} " + "--download` first." ) tail = "" else: @@ -312,6 +374,7 @@ def crop( "count": len(crops), "directory": str(landed), "crops": crops, + **({"downloaded": downloaded} if downloaded is not None else {}), **({"warning": drift} if drift else {}), } if not batch: diff --git a/src/ade_cli/help.py b/src/ade_cli/help.py index 2f33319..2fdacbe 100644 --- a/src/ade_cli/help.py +++ b/src/ade_cli/help.py @@ -252,8 +252,12 @@ ("built", "true when this run rebuilt the artifact"), ("pages_embedded", "pages inlined; the rest load from sidecars"), ("note", "why the render weakened, when it did (else null)"), - ("downloaded", "with --download: true when this run fetched " - "the URL document into the job item (false: already attached)"), + ("downloaded", "URL items: true when this run fetched the " + "document into the job item (automatic on first view; false: " + "already attached, or the automatic fetch failed — see " + "download_error)"), + ("download_error", "why the automatic fetch failed, when it " + "did (the viewer still builds, previews empty; else absent)"), ("deep_link", "view.html#element=... when --element-id was given"), ("history_items", "items in the rebuilt sidebar read model"), ("sidebar_sync", "true when sibling viewers build in the background"), @@ -490,8 +494,9 @@ { "path": " document.", "what": "URL parses: the attached document copy (`parse " - "--keep-copy` / `view --download`) page previews and crops render " - "from — unverified against the parsed run", + "--keep-copy`, or fetched automatically on first `view`/`crop`) " + "page previews and crops render from — unverified against the " + "parsed run", }, { "path": " view.html / crops/", diff --git a/src/ade_cli/parse.py b/src/ade_cli/parse.py index 273a112..f9a6642 100644 --- a/src/ade_cli/parse.py +++ b/src/ade_cli/parse.py @@ -157,9 +157,9 @@ def parse( help="--document-url only: also download the document into the " "job item (plain HTTP, no API credits) so page previews and " "crops render locally — fetched now, while the URL (often " - "pre-signed) still works. Without it, URL parses have no local " - "bytes and the viewer explains how to fetch them later " - "(`ade view --download`).", + "pre-signed) still works. Without it, the first `view`/`crop` " + "fetches the copy instead, by which time a pre-signed URL may " + "have expired.", ), include: list[Include] = typer.Option( [], @@ -324,8 +324,8 @@ def emit_summary( # a warning with the later remediation, never a failure. copy_line = ( f"\n copy: keep-copy failed — {copy_info['error']} " - f"(the parse succeeded; `ade view {ref} --download` " - "can fetch the copy later)" + f"(the parse succeeded; `ade view {ref}` retries the " + "fetch automatically)" ) else: copy_line = ( diff --git a/src/ade_cli/view.py b/src/ade_cli/view.py index a574cd9..6fe4793 100644 --- a/src/ade_cli/view.py +++ b/src/ade_cli/view.py @@ -183,7 +183,7 @@ def _referencing_extractions( def _imagery_source(store: JobStore, bundle: dict) -> str | None: """What this bundle's page imagery renders from: the parse item's recorded source, or its attached copy for URL parses (#169 — - `parse --keep-copy` / `view --download`).""" + `parse --keep-copy`, or fetched on first `view`/`crop`).""" owner = bundle.get("parse_item_id") or bundle["record"]["job_item_id"] return attach.renderable_source(store, owner, bundle["parse_meta"]) @@ -642,13 +642,15 @@ def _build( banner_note = note # A URL item's missing preview gets the id-bearing action (#169): the # cause comes from the raster layer; the command that fixes it needs - # the item id, which only this layer holds. + # the item id, which only this layer holds. The fetch is automatic + # now, so this note survives only a failed or suppressed download — + # the action is a retry, not a flag to discover. if note and note.startswith("source unavailable: parsed from a URL"): owner_id = bundle.get("parse_item_id") or item_id action = ( - f" Fetch them with `ade view {items.short_id(store, owner_id)} " - "--download`, or keep a copy at parse time with " - "`parse --keep-copy`." + f" Re-run `ade view {items.short_id(store, owner_id)} " + "--download` to fetch them, or keep a copy at parse time " + "with `parse --keep-copy`." ) note += action banner_note = note @@ -985,14 +987,18 @@ def view( pages: str | None = typer.Option( None, "--pages", help="Pages to embed images for, 1-indexed, e.g. '1,3-5'." ), - download: bool = typer.Option( - False, "--download", + download: bool | None = typer.Option( + None, "--download/--no-download", help="URL-parsed items: fetch the document from its recorded URL " "into the job item and render page previews from that copy — " - "the parse itself never gives the CLI the bytes (#169). Plain " - "HTTP, no API credits; the copy is unverified against the " - "parsed run. Also works on an extract item id (fetches into " - "its referenced parse item).", + "the parse itself never gives the CLI the bytes (#169). This " + "happens automatically when no copy is attached yet (a notice " + "and progress line land on stderr); --no-download skips the " + "fetch and previews stay empty. Explicit --download makes a " + "failed fetch an error instead of a warning. Plain HTTP, no API " + "credits; the copy is unverified against the parsed run. Also " + "works on an extract item id (fetches into its referenced parse " + "item).", ), no_sidebar_sync: bool = typer.Option( False, "--no-sidebar-sync", @@ -1112,12 +1118,19 @@ def view( record = items.item_record(store, item_id) - # --download (#169): attach the URL document's bytes to the imagery - # owner (the item itself, or a referencing extract's parse item) - # BEFORE the bundle loads, so this same run renders from the copy. + # Attaching the URL document's bytes happens on the imagery owner + # (the item itself, or a referencing extract's parse item) BEFORE the + # bundle loads, so this same run renders from the copy. Default is + # AUTO (#169 follow-up): a URL-sourced item with no attached copy + # fetches now — announced on stderr, never silently — because an + # empty preview surprised users more than an implicit fetch; + # --no-download suppresses. Explicit --download keeps the strict + # contract: wrong-kind items and failed fetches are errors, and the + # already-attached case still gets its receipt line. download_line = "" downloaded: bool | None = None - if download: + download_error: str | None = None + if download is not False: if record["kind"] == "parse": owner_id = item_id else: @@ -1128,7 +1141,8 @@ def view( owner_meta = ( store.read_json(owner_id, "meta.json") if owner_id else None ) - if not owner_id or not attach.is_url_source(owner_meta): + url_source = owner_id is not None and attach.is_url_source(owner_meta) + if download and not url_source: message = ( f"Job item {item_id} has no URL source to download: " "--download applies to items parsed from --document-url " @@ -1144,38 +1158,54 @@ def view( as_json=as_json, code=EXIT_USAGE, ) - already = attach.attached_file(store, owner_id, owner_meta) - if already is not None: + already = ( + attach.attached_file(store, owner_id, owner_meta) + if url_source + else None + ) + if download and already is not None: downloaded = False download_line = ( f"\n download: copy already attached ({already.name}); " "previews render from it" ) - else: + elif url_source and already is None: try: - name, size = attach.download( + name, size = attach.download_with_notice( store, owner_id, owner_meta or {}, - transport=ports.transport, - now=ports.clock.now(), + ports=ports, + as_json=as_json, ) except attach.AttachError as error: - exit_with( - { - "error": error.kind, - "job_item_id": owner_id, - "message": error.message, - }, - error.message, - as_json=as_json, - code=EXIT_FAILED, + if download: + exit_with( + { + "error": error.kind, + "job_item_id": owner_id, + "message": error.message, + }, + error.message, + as_json=as_json, + code=EXIT_FAILED, + ) + # Auto mode degrades instead of failing: yesterday's + # working `ade view` must not start exiting non-zero the + # day the pre-signed URL expires — the viewer still + # builds, with the honest empty-preview note. + downloaded = False + download_error = error.message + download_line = ( + "\n download: failed — page previews stay empty " + f"({error.message})" + ) + else: + downloaded = True + download_line = ( + f"\n download: fetched {name} ({size:,} bytes) into " + f"job item {owner_id}" ) - downloaded = True - download_line = ( - f"\n download: fetched {name} ({size:,} bytes) into job " - f"item {owner_id}" - ) try: bundle = _load_bundle(store, item_id) @@ -1223,8 +1253,8 @@ def view( if "parsed from a URL" in message: owner_id = bundle.get("parse_item_id") or item_id message += ( - f" Fetch it with `ade view {owner_id} --download`, " - "then re-run." + f" Re-run `ade view {owner_id} --download` to fetch " + "it, then re-run this crop." ) tail = "" else: @@ -1381,6 +1411,8 @@ def view( } if downloaded is not None: payload["downloaded"] = downloaded + if download_error is not None: + payload["download_error"] = download_error hint = f"ade view {items.short_id(store, item_id)} --open" + ( f" --element-id {element_id}" if element_id else "" ) diff --git a/tests/test_crop.py b/tests/test_crop.py index 162fdf5..8420fb5 100644 --- a/tests/test_crop.py +++ b/tests/test_crop.py @@ -598,9 +598,8 @@ def test_single_crop_output_path_creates_missing_parents(cli, parsed, tmp_path): assert payload["directory"] == str(out.parent) -def test_crop_of_a_url_parsed_item_names_the_remediation(cli): - """#169's crop face: a URL parse has no local bytes to crop from — the - error must say why and how to get crops, not claim a file vanished.""" +def url_parsed(cli): + """Seed a URL-sourced parse item through the seam; returns its id.""" cli.transport.respond(202, {"job_id": "job-0001"}) cli.transport.respond(200, completed_job(rich_parse_response())) parsed = cli.invoke( @@ -608,10 +607,71 @@ def test_crop_of_a_url_parsed_item_names_the_remediation(cli): "--json", env=AUTH_ENV, ) assert parsed.exit_code == 0, parsed.stdout - item_id = json.loads(parsed.stdout)["job_item_id"] + return json.loads(parsed.stdout)["job_item_id"] - payload = crop_json(cli, item_id, "--element-id", "text-0", exit_code=1) + +def test_crop_of_a_url_parsed_item_names_the_remediation(cli): + """#169's crop face: a URL parse with the fetch suppressed has no + local bytes to crop from — the error must say why and how to get + crops, not claim a file vanished.""" + item_id = url_parsed(cli) + + payload = crop_json( + cli, item_id, "--element-id", "text-0", "--no-download", exit_code=1 + ) assert payload["error"] == "source_missing" assert "parsed from a URL" in payload["message"] + assert "--no-download" in payload["message"] # the suppressed fetch assert "ade parse -d" in payload["message"] + + +def test_crop_auto_downloads_the_url_document(cli, pdf): + """A plain `ade crop` on a URL-sourced item fetches the copy by + default (mirroring `view`) and crops from it in the same run, with + the unverified-bytes caveat as its warning.""" + import httpx + + item_id = url_parsed(cli) + body = pdf.read_bytes() + cli.transport.respond_with(lambda req: httpx.Response(200, content=body)) + + payload = crop_json(cli, item_id, "--element-id", "text-0") + + assert payload["status"] == "cropped" + assert payload["downloaded"] is True + assert "downloaded copy" in payload["warning"] + assert str(cli.transport.requests[-1].url) == "https://example.com/doc.pdf" + assert (cli.home / "jobs" / item_id / "document.pdf").read_bytes() == body + + # The copy attached: the next crop has nothing to fetch. + seen = len(cli.transport.requests) + again = crop_json(cli, item_id, "--element-id", "text-0") + assert len(cli.transport.requests) == seen + assert "downloaded" not in again + + +def test_crop_auto_download_failure_is_the_crops_failure(cli): + """Unlike `view` there is no degraded render to fall back to — an + expired URL fails the crop with the download error, which is exactly + what the no-copy state already meant.""" + import httpx + + item_id = url_parsed(cli) + cli.transport.respond_with(lambda req: httpx.Response(403, content=b"denied")) + + payload = crop_json(cli, item_id, "--element-id", "text-0", exit_code=1) + + assert payload["error"] == "download_failed" + assert "expire" in payload["message"] + assert not (cli.home / "jobs" / item_id / "document.pdf").exists() + + +def test_crop_download_refuses_a_local_source_item(cli, parsed): + item_id, _ = parsed + + payload = crop_json( + cli, item_id, "--element-id", "text-0", "--download", exit_code=2 + ) + + assert payload["error"] == "not_a_url_source" diff --git a/tests/test_serve.py b/tests/test_serve.py index 249b42c..ebd86d3 100644 --- a/tests/test_serve.py +++ b/tests/test_serve.py @@ -42,7 +42,7 @@ def test_serve_reuses_a_running_server(cli, parsed_url_item, monkeypatch): "ade_cli.view._spawn_server", lambda port: pytest.fail("a live server must be reused, not respawned"), ) - payload = view_json(cli, item_id, "--serve") + payload = view_json(cli, item_id, "--serve", "--no-download") assert payload["url"] == f"http://127.0.0.1:8642/jobs/{item_id}/view.html" assert payload["serve_error"] is None probe = cli.transport.requests[-1] @@ -64,7 +64,7 @@ def fake_spawn(port): cli.transport.respond(200, _health_body(cli)) monkeypatch.setattr("ade_cli.view._spawn_server", fake_spawn) - payload = view_json(cli, item_id, "--serve") + payload = view_json(cli, item_id, "--serve", "--no-download") assert spawned == [serve.DEFAULT_PORT] assert payload["url"] == f"http://127.0.0.1:8644/jobs/{item_id}/view.html" assert payload["deep_link"] is None @@ -84,7 +84,7 @@ def fake_spawn(port): cli.transport.respond(200, _health_body(cli)) monkeypatch.setattr("ade_cli.view._spawn_server", fake_spawn) - payload = view_json(cli, item_id, "--serve") + payload = view_json(cli, item_id, "--serve", "--no-download") assert payload["url"] == f"http://127.0.0.1:8645/jobs/{item_id}/view.html" @@ -95,7 +95,8 @@ def test_serve_failure_degrades_to_file(cli, parsed_url_item, monkeypatch): monkeypatch.setattr("ade_cli.view._spawn_server", lambda port: None) opened = [] result = cli.invoke( - "view", item_id, "--serve", "--open", "--no-sidebar-sync", "--json", + "view", item_id, "--serve", "--open", "--no-sidebar-sync", "--no-download", + "--json", browser=lambda url: opened.append(url) or True, ) assert result.exit_code == 0, result.stdout @@ -116,7 +117,7 @@ def broken_spawn(port): raise OSError("posix_spawn failed") monkeypatch.setattr("ade_cli.view._spawn_server", broken_spawn) - payload = view_json(cli, item_id, "--serve") + payload = view_json(cli, item_id, "--serve", "--no-download") assert payload["url"] is None assert "could not start the viewer server" in payload["serve_error"] assert payload["path"].endswith("view.html") @@ -132,6 +133,7 @@ def test_serve_url_is_the_browser_target_with_deep_link( opened = [] result = cli.invoke( "view", item_id, "--serve", "--open", "--element-id", "text-0", + "--no-download", "--no-sidebar-sync", "--json", browser=lambda url: opened.append(url) or True, ) @@ -147,7 +149,7 @@ def test_daemon_serves_artifacts_health_and_refuses_listings(cli, parsed_url_ite """The real handler on a real loopback socket: health names the store, artifacts stream, directory listings are refused (ADR-0005).""" item_id = parsed_url_item - view_json(cli, item_id) # build the artifact the server will serve + view_json(cli, item_id, "--no-download") # build what the server serves server = serve._bind(cli.home, 0) # candidate 0 = OS-assigned, race-free port = server.server_address[1] thread = threading.Thread(target=server.serve_forever, daemon=True) diff --git a/tests/test_view.py b/tests/test_view.py index 3f71c68..a005048 100644 --- a/tests/test_view.py +++ b/tests/test_view.py @@ -608,7 +608,7 @@ def test_view_never_reuses_an_unrenderable_build(cli, tmp_path): def test_view_degrades_for_url_documents(cli): item_id = seed_parse_item(cli, url="https://example.com/invoice.pdf") - payload = view_json(cli, item_id) + payload = view_json(cli, item_id, "--no-download") assert payload["built"] is True assert "URL" in payload["note"] @@ -784,7 +784,7 @@ def test_history_js_neutralizes_script_closing_sequences(cli): # history.js injection posture). item_id = seed_parse_item(cli, url="https://x.test/") - view_json(cli, item_id) + view_json(cli, item_id, "--no-download") raw = (cli.home / "history.js").read_text(encoding="utf-8") assert " Date: Thu, 6 Aug 2026 12:11:52 +0800 Subject: [PATCH 2/3] fix: document crop's downloaded key and when view omits it (Copilot review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The crop payload reference was missing the new `downloaded` member, and view's description implied `false` covers the already-attached case — auto mode omits the key entirely there; false means a failed automatic fetch or explicit --download finding an existing copy. Co-Authored-By: Claude Fable 5 --- docs/reference/help.json | 6 +++++- src/ade_cli/help.py | 10 +++++++--- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/docs/reference/help.json b/docs/reference/help.json index d058fb7..659ca17 100644 --- a/docs/reference/help.json +++ b/docs/reference/help.json @@ -1061,7 +1061,7 @@ }, { "key": "downloaded", - "what": "URL items: true when this run fetched the document into the job item (automatic on first view; false: already attached, or the automatic fetch failed \u2014 see download_error)" + "what": "URL items: true when this run fetched the document into the job item (automatic on first view). false when the automatic fetch failed (see download_error) or explicit --download found the copy already attached; absent when nothing needed fetching" }, { "key": "download_error", @@ -1175,6 +1175,10 @@ { "key": "crops", "what": "one record per PNG (element_id, type, page, box, dpi, path, width, height)" + }, + { + "key": "downloaded", + "what": "URL items: true when this run fetched the document into the parse item before cropping (absent when nothing needed fetching)" } ], "note": "One shape whatever matched: a single --element-id is count 1 with one crops[] record; a filter (--type/--page/--all) matching nothing is count 0 with crops []." diff --git a/src/ade_cli/help.py b/src/ade_cli/help.py index 2fdacbe..4688132 100644 --- a/src/ade_cli/help.py +++ b/src/ade_cli/help.py @@ -237,6 +237,9 @@ ("directory", "where they landed"), ("crops", "one record per PNG (element_id, type, " "page, box, dpi, path, width, height)"), + ("downloaded", "URL items: true when this run fetched the " + "document into the parse item before cropping (absent when " + "nothing needed fetching)"), ], "note": "One shape whatever matched: a single --element-id is " "count 1 with one crops[] record; a filter (--type/--page/--all) " @@ -253,9 +256,10 @@ ("pages_embedded", "pages inlined; the rest load from sidecars"), ("note", "why the render weakened, when it did (else null)"), ("downloaded", "URL items: true when this run fetched the " - "document into the job item (automatic on first view; false: " - "already attached, or the automatic fetch failed — see " - "download_error)"), + "document into the job item (automatic on first view). false " + "when the automatic fetch failed (see download_error) or " + "explicit --download found the copy already attached; absent " + "when nothing needed fetching"), ("download_error", "why the automatic fetch failed, when it " "did (the viewer still builds, previews empty; else absent)"), ("deep_link", "view.html#element=... when --element-id was given"), From f76c61f46310171ab26e055b703db69950751f04 Mon Sep 17 00:00:00 2001 From: Yuanwen Tian Date: Thu, 6 Aug 2026 12:48:16 +0800 Subject: [PATCH 3/3] fix: update SKILL.md's URL-parse note for the auto-download default Co-Authored-By: Claude Fable 5 --- SKILL.md | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/SKILL.md b/SKILL.md index 14c56ac..0e8fc2d 100644 --- a/SKILL.md +++ b/SKILL.md @@ -184,8 +184,10 @@ clearing a parse item cascades to the extractions referencing it. - **`--markdown` extractions have no page evidence** (there is no parse to join against) — evidence degrades to spans-only, and `view` renders the markdown pane alone. -- **URL parses have no local bytes**, so `view`/`crop` cannot render - page imagery until you attach a copy: `parse --document-url … - --keep-copy` at parse time (reliable — pre-signed URLs expire), or - `ade view --download` after the fact. Markdown, elements, and - extractions work either way. +- **URL parses have no local bytes**, so page imagery renders from an + attached copy: `parse --document-url … --keep-copy` fetches it at + parse time (reliable — pre-signed URLs expire); otherwise the first + `view`/`crop` downloads it automatically (`--no-download` skips; + the payload records `downloaded`, and on a failed fetch `view` + degrades to an empty preview with `download_error` while `crop` + errors). Markdown, elements, and extractions work either way.