From 17fdf8fd7beb353953ef0e8b18f227de3a515500 Mon Sep 17 00:00:00 2001 From: he Date: Fri, 4 Sep 2026 16:12:55 -0400 Subject: [PATCH] fix: pos ai hf download --gguf crashes on tree API responses The HF tree API returns entries shaped {oid,path,size,type} with no rfilename field, so every downstream .rfilename read was null: the --gguf filter crashed with 'jq: endswith() requires string inputs' and single-file/all-files/meta modes silently built 'null' URLs. hf_repo_files now normalizes tree entries to the {rfilename,size} shape the fallback already emits (object-guarded; error-object bodies degrade to [] instead of jq 5). The --gguf filter is type-guarded and empty results get mode-aware messages. Verified: 12/12 fixture harness, live API 13->10 gguf, tiny real download OK, gates green. User confirmed the real --gguf command now downloads [1/10]. --- AGENT_TODO.md | 4 + .../2026-09-04_pos-ai-hf-gguf-jq-bug-fix.md | 120 +++++++++++ .../2026-09-04_pos-ai-hf-gguf-jq-bug.md | 196 ++++++++++++++++++ ...2026-09-04_pos-ai-hf-gguf-jq-bug-review.md | 118 +++++++++++ DOC/AGENT_Context_Project.md | 2 +- bin/pos-ai-hf | 19 +- 6 files changed, 454 insertions(+), 5 deletions(-) create mode 100644 AgentsReport/builder/2026-09-04_pos-ai-hf-gguf-jq-bug-fix.md create mode 100644 AgentsReport/detective/2026-09-04_pos-ai-hf-gguf-jq-bug.md create mode 100644 AgentsReport/reviewer/2026-09-04_pos-ai-hf-gguf-jq-bug-review.md diff --git a/AGENT_TODO.md b/AGENT_TODO.md index 3c9a7ba..e5612a7 100644 --- a/AGENT_TODO.md +++ b/AGENT_TODO.md @@ -42,6 +42,10 @@ summary (newest last). ## Done +- **2026-09-04** — Fix `pos ai hf download --gguf` crashing with `jq: error: endswith() requires string inputs` (user report). Root cause: `hf_repo_files()` primary path returned the RAW HF tree API response (`{oid,path,size,type}` — no `rfilename` field), so `.rfilename` was null for every entry; the `--gguf` filter `endswith(.rfilename)` crashed, and single-file/all-files/meta/summary modes were silently broken too (built URLs with literal "null"). Fix: normalize the tree response to `[.[] | select(type == "object" and .type == "file") | {rfilename: .path, size: (.size // 0)}]` (same `{rfilename,size}` shape the sibling fallback already emits — hardened against error-object bodies: `{"error":…}` → `[]` rc 0, was rc 5); `--gguf` filter gains a `type == "string"` guard; empty results get mode-aware messages (" not found in ", "No .gguf files found in — try without --gguf", "No files to download"). Verified: fixture harness `/tmp/opencode/hf-test2/run-tests.sh` 12/12 green; live API: normalize → 13 records / 0 nulls, `--gguf` → exactly 10 .gguf (no README/LICENSE/.gitattributes); tiny real download (`download Qwen/… LICENSE`) OK; user confirmed the full `--gguf` command now downloads `[1/10] …`; `bash -n`; `make gen && make check` green; `make lint` 0 FAIL / 0 WARN. Chain: Detective (root cause + sweep) → Builder (3-hunk fix + hardening) → Reviewer APPROVE_WITH_NOTES. + +- **2026-09-04** — Fix `pos media ytsync add <@handle>` treating a channel's tabs as videos (live user report): bare channel URLs (`@handle`, `/c/`, `/user/`, `/channel/ID`, `music.youtube.com/channel/ID`) return the channel's tab structure (Videos/Live/Shorts — `_type:"playlist"`, `url:null`, `id==channel_id`) in `yt-dlp --flat-playlist` mode, so ytsync tried to download the channel ID as a video and failed with "This video is unavailable". Fix: probe-time canonicalization — new `canonical_channel_url()` called at the top of `run_probe()` appends `/videos` to bare channel URLs (works for `add` AND `sync` of already-stored bare-handle registry entries, no migration; explicit tabs `/videos|shorts|streams|live|playlists|featured|…` untouched; `?v=`/`?list=`/`youtu.be` untouched); `collect_entries()` filters to watchable entries (`watch?v=|youtu.be/|/shorts/`) with a `_type=="video"` fallback guard so empty channels degrade to graceful 0-new. Live: `sync --dry-run` now resolves `3Blue1Brown (channel · 151 videos)` with real titles. Verified: stub harness `/tmp/opencode/ytsync-test/run-tests.sh` 32/32 green; `bash -n`; `make gen && make check` green; `make lint` 0 FAIL / 0 WARN. Chain: Detective (root cause + spec) → Builder → Reviewer APPROVE_WITH_NOTES. + - **2026-09-04** — `pos ai hf` (`bin/pos-ai-hf`) — Hugging Face model downloader. Subcommands: `download [filename]` (single file, whole repo, `--gguf` filter, `--branch `, `--output `), `search `, `list`, `remove`. Downloads to `~/.local/share/linux_post_install/ai/models/-/` (seam-guarded `HF_DOWNLOAD_DIR`), writes `.hf-meta` JSON per repo, prints structured summary (📥/📁). Config extends the existing `ai` scope via `# POS_CONFIG: ai` — `HF_TOKEN` (secret) and `HF_DOWNLOAD_DIR` in `~/.config/linux_post_install/ai.env` with env-var precedence. Auth on all requests; HTTP 429 rate-limit sleep + retry once; resume via `curl -C -`; progress bars to stderr. Deps: `curl`/`jq` guards before `--help`; no stdin → not in INTERACTIVE_CMDS. Verified: stub-PATH suite `/tmp/opencode/hf-test/run-tests.sh` 46/46 green (argument parsing, download single/multi/gguf/branch/output, search, list, remove, config/token, output format); `bash -n`; `make gen && make check` green; `make lint` 0 FAIL / 0 WARN. Docs: POS.md ai row + detail block. - **2026-09-04** — `pos media grab` (`bin/pos-media-grab`) — auto-download a URL as audio or video. Classifies by domain (YouTube Music/SoundCloud/Bandcamp → mp3; YouTube/Vimeo/Twitch → mp4) with `--audio`/`--video` overrides and `GRAB_DEFAULT` config (`pos config grab`, default `video`) for unknown domains; `--best` default for video (non-interactive, `--worst` override); all flags (`--output`, `--no-playlist`, `--cookies`, `--dry-run`) forwarded to mp3/mp4; prints a clean summary (🎵/🎬 title, duration, path, size). Telegram listener (`bin/pos-communication-telegram-listener`) gains `url_detect` + a URL routing step between the prefix map and AI bridge — bare http(s) URLs route to `pos media grab --best` (600s timeout). Verified: `/tmp/opencode/media-grab-test/run-tests.sh` 28 cases / 70 assertions green; `bash -n` on both files; `make gen && make check` green; `make lint` 0 FAIL / 0 WARN. diff --git a/AgentsReport/builder/2026-09-04_pos-ai-hf-gguf-jq-bug-fix.md b/AgentsReport/builder/2026-09-04_pos-ai-hf-gguf-jq-bug-fix.md new file mode 100644 index 0000000..a20dacc --- /dev/null +++ b/AgentsReport/builder/2026-09-04_pos-ai-hf-gguf-jq-bug-fix.md @@ -0,0 +1,120 @@ +# Builder Report — `pos-ai-hf` GGUF/jq bug fix (2026-09-04) + +## TL;DR + +- **Status:** IMPLEMENTED +- **Scope:** fix `jq: endswith() requires string inputs` crash in `pos ai hf download … --gguf` per Detective spec (AgentsReport/detective/2026-09-04_pos-ai-hf-gguf-jq-bug.md, Step 4); Changes 1-3 implemented exactly; harness at `/tmp/opencode/hf-test2/`; no commit/push. +- **Files changed (repo):** `bin/pos-ai-hf` (+17/-4); `DOC/AGENT_Context_Project.md` (2-line auto-gen filetable row, only the `pos-ai-hf` line count 495→506 — `make gen` output; no GEN block content changed). +- **Verification:** `bash -n` OK; harness 9/9 PASS; `make gen/check/lint` green (`0 FAIL, 0 WARN`); live API: normalize→13 records (0 nulls, 0 bad sizes), --gguf→exactly 10, README/LICENSE/.gitattributes excluded; real tool path: single-file `LICENSE` download OK (7.2 KB, `.hf-meta` correct); mode-aware empty messages verified live (exit 1 unchanged). +- **NOT committed.** Working-tree changes: `bin/pos-ai-hf`, `DOC/AGENT_Context_Project.md` (+1/-1 line-count row), untracked Detective report (pre-existing). + +## Step 1: Read Detective report + confirm scope — [DONE] + +Read full report (fix spec Step 4, edge cases Step 5, harness spec Step 6, verification Step 7). Confirmed fixtures exist: `/tmp/opencode/qwen-tree.json` (13 files, no rfilename), `/tmp/opencode/sd-tree.json` (8 files + 7 dirs). + +## Step 2: Implement Change 1 — normalize tree response in `hf_repo_files()` — [DONE] + +`bin/pos-ai-hf:201-207` — primary `/tree` path now pipes through the normalize jq instead of echoing raw: + +```bash +# Tree API returns {type,path,size,oid[,lfs]} per entry — normalize to the +# {rfilename,size} shape the rest of the pipeline expects (same as fallback). +# Skip "directory" entries: they have no resolvable file URL. +printf '%s' "$result" | jq '[.[] | select(.type == "file") | {rfilename: .path, size: (.size // 0)}]' +``` + +Fallback sibling path (line ~213) untouched — already emits `{rfilename, size}`. + +## Step 3: Implement Change 2 — defense-in-depth guard in `--gguf` filter — [DONE] + +`bin/pos-ai-hf:339`: + +```bash +filtered_files="$(printf '%s' "$files_json" | jq -c '[.[] | select((.rfilename | type) == "string" and (.rfilename | endswith(".gguf")))]')" +``` + +## Step 4: Implement Change 3 — mode-aware empty results — [DONE] + +`bin/pos-ai-hf:347-356` — replaced `[ "$file_count" -gt 0 ] || err "No files to download"` with `if [ "$file_count" -eq 0 ]` branch: +- single-file mode: `err "File not found: $filename in $repo_id (branch: ${branch})"` +- `--gguf` mode: `err "No .gguf files found in $repo_id${branch:+ (branch: $branch)} — try without --gguf"` +- generic: `err "No files to download"` (unchanged text) + +Exit semantics unchanged (same `err` path, exit 1). + +## Step 5: Build harness `/tmp/opencode/hf-test2/` — [DONE] + +- `fixtures/tree-files.json` = copy of `/tmp/opencode/qwen-tree.json` (13 files, rfilename ABSENT, 10 .gguf) +- `fixtures/tree-with-dirs.json` = copy of `/tmp/opencode/sd-tree.json` (8 files + 7 `type:"directory"`) +- `fixtures/tree-empty.json` = `[]` +- `fixtures/tree-nogguf.json` = `[{"type":"file","path":"README.md","size":100}]` +- `run-tests.sh`: 9 assertions per report Step 6 (t_tree_normalize, t_gguf_on_normalized, t_single_file, t_all_files_passthrough, t_empty, t_nogguf, t_defense_guard, t_dirs_excluded, t_code_sync). Deliberately does NOT `source` bin/pos-ai-hf (top-level dispatch executes; no-args → usage → exit 0). NORM/GGUF_FILTER/FN_FILTER duplicated verbatim; `grep -F` drift-guards catch divergence from the file. + +## Step 6: Verification budget — [DONE] + +1. `bash -n bin/pos-ai-hf` → OK +2. `bash /tmp/opencode/hf-test2/run-tests.sh` → **9 passed, 0 failed** (output captured in Step 5 run) +3. `make gen && make check && make lint` → gen OK, check-sync OK, lint **0 FAIL, 0 WARN**; regenerated: `DOC/AGENT_Context_Project.md` line-count row only (495→506); no GEN:START/END block changes +4. LIVE validation (no download): fetched `https://huggingface.co/api/models/Qwen/Qwen2.5-3B-Instruct-GGUF/tree/main`; normalize jq → `{count:13, nulls:0, badsizes:0}`; --gguf filter → length 10; paths = 10 `qwen2.5-3b-instruct-*.gguf` (fp16×2 + q2_k..q8_0); README/LICENSE/.gitattributes excluded; pre-fix crash `endswith() requires string inputs` reproduced for contrast on raw tree +5. Real tool-path proof (no multi-GB download): + - `pos ai hf download Qwen/Qwen2.5-3B-Instruct-GGUF LICENSE --output /tmp/hf-small` → exit 0, `📥 Downloaded: Qwen/Qwen2.5-3B-Instruct-GGUF/LICENSE (7.2 KB)`, `.hf-meta` `"files": ["LICENSE"]`, real 7388-byte file present. NOTE: first attempt via `bin/pos` hit the stale **installed** `/usr/local/bin/pos-ai-hf` (PATH precedence) which silently no-matched → reproduced the pre-fix bug; installed copy is now byte-identical to repo and the same command succeeds. Environment detail, not a code issue. + - `--gguf` no-gguf repo: `… distilbert/distilbert-base-uncased --gguf` → `ERROR: No .gguf files found in distilbert/distilbert-base-uncased (branch: main) — try without --gguf`, exit 1 (no crash, no download) + - single-file not-found: `… totally-missing-file` → `ERROR: File not found: totally-missing-file in Qwen/Qwen2.5-3B-Instruct-GGUF (branch: main)`, exit 1 + +## Step 7: Final diff review + handoff — [DONE] + +`git diff --stat`: `bin/pos-ai-hf | 17 ++++-----` (only the 3 sanctioned hunks), `DOC/AGENT_Context_Project.md | 2 +-` (gen line-count row). `git status`: no staged/committed changes; NOT committed or pushed. + +## Handoff + +Status: **IMPLEMENTED** + +Approved scope: the 3 changes in Detective fix spec Step 4; harness at `/tmp/opencode/hf-test2/`; verification per Step 7. Only `bin/pos-ai-hf` changed in repo (+ `make gen` line-count row in DOC/AGENT_Context_Project.md). + +Remaining risks / follow-ups (out of scope, flagged by Detective): +- `hf_api` lacks `curl -L` → 307-redirect alias repos (e.g. runwayml) still fail (pre-existing). +- Tree endpoint non-recursive → subdirectory files not listed (pre-existing semantics). +- `/usr/local/bin` installed copy was stale at run time (auto-synced later); real deployers should reinstall. + +Recommended next agent: **Tester** — the /tmp/opencode/hf-test2 harness is fixture-based and ready for adoption into the repo test suite if the project chooses (decision: Architect); otherwise Reviewer for acceptance of the 3-hunk fix. + +Changes made by Builder: as listed above; nothing else touched. + +## Harden+verification — error-object hardening (2026-09-04, Orchestrator follow-up) — [DONE] + +Previous implementation APPROVED. Orchestrator/Reviewer found an additional crash class: `printf '%s' '{"error":"x"}' | jq '[.[] | select(.type == "file") | …]'` → `jq: error: Cannot index string with string "type"` rc 5 — `.[]` on an object iterates its VALUES; the string `"x"` then gets indexed with `.type`. Verified present on BOTH normalize paths pre-change (primary: `Cannot index string…`; fallback: `Cannot iterate over null (null)` on `.siblings`). + +### Change 4 — object-safe normalize (primary), object-safe fallback (new) + +`bin/pos-ai-hf:205` (primary, now object-guarded; jq `and` short-circuits so `.type` is never evaluated on non-objects): + +```bash +printf '%s' "$result" | jq '[.[] | select(type == "object" and .type == "file") | {rfilename: .path, size: (.size // 0)}]' +``` + +`bin/pos-ai-hf:213` (fallback, previously unguarded — same crash class; now `[]?` suppresses null iteration + `select(type == "object")` skips junk elements + `(rfilename // "")` keeps the shape contract string-safe for nulls): + +```bash +printf '%s' "$fallback" | jq '[.siblings[]? | select(type == "object") | {rfilename: (.rfilename // ""), size: (.size // 0)}]' +``` + +- On `{"error":"x"}`: primary → `[]` rc 0; fallback → `[]` rc 0 (both were rc 5 before). +- On real fixtures: 13-file tree → 13 records, 0 nulls (identical to pre-hardening); dirs fixture → 8 (dirs dropped); real metadata qwen-meta.json → 13 records, string rfilename, numeric size. +- Pathological `{"rfilename":42}` in siblings passes `// ""` unchanged (42 is truthy → kept) — non-null non-string rfilename still possible in the fallback shape; the `--gguf` filter's `type == "string"` guard prevents the crash class there, and single-file select simply won't match. Flagged as accepted residual risk (suggested-form semantics per Orchestrator). +- Line count unchanged (506) → `make gen` produced no further DOC change beyond the already-tracked 495→506 line-count row. + +### Harness update + +`/tmp/opencode/hf-test2/run-tests.sh`: +- `NORM` updated to hardened primary form; new `FB_NORM` duplicated verbatim. +- New fixture `fixtures/meta-siblings.json` = copy of `/tmp/opencode/qwen-meta.json` (13 siblings, real metadata shape). +- New assertions: `t_error_object_normalize` (`{"error":"x"}` → `[]` rc 0), `t_fallback_normalize` (real metadata → 13 records, string rfilename, numeric size), `t_error_object_fallback` (`{"error":"x"}` → `[]` rc 0). +- `t_code_sync` drift-guards updated: greps `select(type == "object" and .type == "file")` (primary) and `select(type == "object")` (fallback) in addition to the GGUF guard + fn filter. + +### Harden verification results + +1. `bash -n bin/pos-ai-hf` → OK +2. `bash /tmp/opencode/hf-test2/run-tests.sh` → **12 passed, 0 failed** (was 9; +3 new assertions) +3. `make gen && make check && make lint` → gen OK, check-sync OK, **0 FAIL, 0 WARN**; `git diff --stat`: `bin/pos-ai-hf | 19 ++++---` (4 sanctioned hunks: normalize + fallback + gguf guard + message branch + comment), `DOC/AGENT_Context_Project.md | 2 +-` (line-count row from prior gen; unchanged by this pass) +4. LIVE (real API, no download): Qwen tree → hardened normalize `{"count":13,"nulls":0}`; hardened `--gguf` filter → 10; README/LICENSE/.gitattributes excluded → `OK` +5. Still NOT committed; only intended files modified (bin/pos-ai-hf, DOC line-count row) + untracked reports. \ No newline at end of file diff --git a/AgentsReport/detective/2026-09-04_pos-ai-hf-gguf-jq-bug.md b/AgentsReport/detective/2026-09-04_pos-ai-hf-gguf-jq-bug.md new file mode 100644 index 0000000..bb18e20 --- /dev/null +++ b/AgentsReport/detective/2026-09-04_pos-ai-hf-gguf-jq-bug.md @@ -0,0 +1,196 @@ +# Detective Report — `pos-ai-hf` GGUF/jq bug (2026-09-04) + +## TL;DR +- **Status:** ROOT_CAUSE_ESTABLISHED +- **Symptom:** `pos ai hf download Qwen/Qwen2.5-3B-Instruct-GGUF --gguf --output ~/.models` crashes with `jq: error (at :0): endswith() requires string inputs` (jq exit 5, after `[!] No HF_TOKEN set` warning). +- **Root cause (FACT):** `hf_repo_files()` primary path (`bin/pos-ai-hf:199-204`) returns the **raw HF tree API response**, whose entries have keys `oid, path, size, type` — **no `rfilename`**. Every consumer of `files_json` reads `.rfilename` → gets `null`. Line 336 (`endswith(".gguf")` on null) is the crash site. **The user's null-guard alone is insufficient**: with the guard, `--gguf` would silently filter everything out → `err "No files to download"` (exit 1) instead of downloading the 10 GGUF files. All other modes are also silently broken for every tree-served repo: single-file mode matches nothing, all-files mode writes `null` into the download URL (404), metadata, and summary. +- **Fix:** normalize the tree response in `hf_repo_files()` to `[.[] | select(.type == "file") | {rfilename: .path, size: (.size // 0)}]` (same shape the fallback already emits), plus a defense-in-depth string guard on the `--gguf` filter and mode-aware empty-result messages. Verified: 13 files normalize, `--gguf` selects the 10 `.gguf` files, single-file/all-files/meta/summary all work unchanged. Live API validation passed (10/10, 0 non-gguf, 0 nulls). +- Expected net change: raw tree result transformed upstream; no semantics change for already-working repos. +- Artifacts: this report; fixtures/harness spec → `/tmp/opencode/hf-test2/` (Builder builds it; harness spec in Step 5). No changes made by Detective (read-only). + +## Step 1: Confirm + quantify the crash and every `.rfilename` read — [DONE] + +Fixture `/tmp/opencode/qwen-tree.json` (live capture of `GET /api/models/Qwen/Qwen2.5-3B-Instruct-GGUF/tree/main`): 13 entries, **all `type:"file"`**, keys per entry `oid, path, size, type`, **no `rfilename`**; 10 entries carry `lfs`. `.path` column: `.gitattributes, LICENSE, README.md, qwen2.5-3b-instruct-{fp16-00001-of-00002,fp16-00002-of-00002,q2_k,q3_k_m,q4_0,q4_k_m,q5_0,q5_k_m,q6_k,q8_0}.gguf`. + +Exact reproduction (the exact code, same exit code as the tool — jq exit 5): +``` +$ jq -c '[.[] | select(.rfilename | endswith(".gguf"))]' /tmp/opencode/qwen-tree.json +jq: error (at qwen-tree.json:0): endswith() requires string inputs # exit=5 +``` + +Every downstream `.rfilename` read, observed (not inferred): + +| Line | Code | Observed with qwen tree | Verdict | +|---|---|---|---| +| 333 | single-file `select(.rfilename == $fn)` | `[]` for any fn (`null == "README.md"` → false; exit 0) | silent no-match → `err "No files to download"` | +| 336 | `--gguf` `select(.rfilename \| endswith(".gguf"))` | **jq error exit 5** (the reported crash) | the crash | +| 339 | all-files `jq -c '.'` | passes all 13 (no filter) | nothing filtered, but downstream 372 breaks | +| 372 | loop `jq -r '.rfilename'` | `null` ×13 | URL `…/resolve/main/null` → 404; `curl -o` left an empty `null` file; loop `warn`ed | +| 398 | meta `[.[] \| .rfilename]` | `[null,null,…13]` | `.hf-meta` `files` list all null | +| 412 | one-file summary `.[0].rfilename` | `null` | `Downloaded: …/null` | + +`jq empty` (line 181), `.size // 0` (lines 373, 414), `[.[].size // 0] \| add // 0` (line 353): unaffected — size handling is already null-safe. + +**Fallback shape verified live** (`GET /api/models/Qwen/Qwen2.5-3B-Instruct-GGUF` → `jq '[.siblings[] | {rfilename: .rfilename, size: (.size // 0)}]'`): 13 entries, 0 null rfilename, all `size: 0` (metadata API has no per-sibling sizes). Shape `{rfilename, size}` — exactly what the normalization produces for the tree path. **Fallback path needs no change.** + +## Step 2: Unsafe-jq sweep (whole file `bin/pos-ai-human`) — [DONE] + +All jq expressions in `bin/pos-ai-hf`, with assessment (only c/p rfilename-related ones are the bug family): + +| Line | Expression | Assessment | +|---|---|---| +| 181 | `jq empty` on API body | validation only; safe | +| 210 | `[.siblings[]\|{rfilename:.rfilename, size:(.size//0)}]` | correct shape; null-safe; **leave as is** | +| 217 | `jq -sRr @uri` (query encode) | safe | +| 254 | `.defaultBranch // empty` | null-safe; safe | +| 298 | `jq 'length'` (search) | safe | +| 302 | `.[] \| "…\(.id)…\(.downloads // 0)…\(.likes // 0)"` | search API provides these; `// 0` guards; safe | +| **333** | `select(.rfilename == $fn)` | **AFFECTED**: null vs string → silently `[]`. Fixed by normalization (works after); no other string-op risk. | +| **336** | `select(.rfilename \| endswith(".gguf"))` | **THE CRASH**. Category (a): string function on possibly-null field. | +| 339 | `jq -c '.'` | passthrough; safe | +| 343 | `jq 'length'` | safe | +| 353 | `[.[].size // 0] \| add // 0` | null-safe on size; safe | +| **372** | `jq -r '.rfilename'` | **Affected**: prints literal `null` → bad URL/404 + empty `null` target file | +| 373 | `jq -r '.size // 0'` | null-safe; safe | +| **398** | `jq -c '[.[] \| .rfilename]'` | **Affected**: meta list all nulls | +| **412** | `jq -r '.[0].rfilename'` | **Affected**: summary prints `null` | +| 414 | `jq -r '.[0].size // 0'` | null-safe; safe | +| 447 | `jq -r '.downloaded_at // "unknown"'` (meta file) | safe; meta file is JSON | + +Category (a) string-function-on-null type: only line 336 in this file (no `startswith`/`contains`/`test` in `pos-ai-hf` at all — grep confirmed; the other matches above are in other tools/service files, out of scope). Category (b) assumes-field-primary-API-returns: only the rfilename family above (lines 333/336/372/398/412). Category (c) covered in Step 1. Category (d) silent no-match on null: line 333 (only one). **No other crash-class bugs found; the rfilename family is the whole story.** + +Related-but-out-of-scope notes (observations, not part of this fix): +- `hf_api` uses `curl -sS` without `-L`; HF redirects some aliases (verified: `runwayml/stable-diffusion-v1-5/tree/main` → 307 → `stable-diffusion-v1-5/stable-diffusion-v1-5`). Such repos fail on BOTH tree and fallback (`API request failed (HTTP 307)`). Pre-existing; unrelated to this bug; would need `-L` or canonical-resolution; flag to Builder/Architect, don't fold in. +- Tree endpoint is non-recursive; repos with subdirectories (e.g. SD-v1-5: `feature_extractor/`, …) only list top-level entries + `type:"directory"` markers. Fix filters out directories → all-files mode skips subdir files (same as pre-bug behavior; tree path never listed them). Optional follow-up: `?recursive=true` — requires `# POS_FLAGS`/docs change, NOT part of this minimal fix. +- `hf_download_file` URL building concatenates raw `path` into URL; files with spaces would need URL-encoding (`@uri`). Pre-existing; rare for models; not this bug. +- Names that are `-`, `.` etc. unaffected. + +## 3. `hf_download_file` / `hf_api` related to THIS bug — [DONE] + +- `hf_download_file` (264-286): no field assumptions of its own; takes URL+target. It is a victim: with raw-tree null rfilename, URL `…/resolve/main/null` returns 404, curl fails → `warn "Download interrupted for null (resume…)"`, and an empty `null` file remains in the model dir (then `cmd_list` counts it as size 0). After normalization the function works as designed (has `-L` for HF's 302→CDN; `-C -` resume; empty-file guard). **No change needed.** +- `hf_api` (134-186): 200/401/403/404/429 handling + `jq empty` validation — nothing rfilename-related. **No change needed** (the 307 note in Step 2 is separate). +- `hf_resolve_branch` (241-261): live-verified defaultBranch "main" resolves fine; unaffected. + +## 4. Fix spec — Builder-executable — [DONE] + +**Objective:** make the primary tree path emit the same `{rfilename, size}` shape the rest of the file (and the fallback path) already assume. Minimal, CLI semantics preserved (all options, filters, messages keep their meaning; only multi-mode empty-result messages get mode-specific text). + +Where: `hf_repo_files()` body, primary branch, current lines 198-204. + +**Change 1 — normalize tree response (the fix).** Replace: +```bash + local endpoint="/models/${ns}/${repo}/tree/${branch}" + local result + if result="$(hf_api "$endpoint" 2>/dev/null)"; then + printf '%s' "$result" + return 0 + fi +``` +with (exact code for Builder): +```bash + local endpoint="/models/${ns}/${repo}/tree/${branch}" + local result + if result="$(hf_api "$endpoint" 2>/dev/null)"; then + # Tree API returns {type,path,size,oid[,lfs]} per entry — normalize to the + # {rfilename,size} shape the rest of the pipeline expects (same as fallback). + # Skip "directory" entries: they have no resolvable file URL. + printf '%s' "$result" | jq '[.[] | select(.type == "file") | {rfilename: .path, size: (.size // 0)}]' + return 0 + fi +``` +- This reuses the exact jq via one pipe; `set -euo pipefail` semantics: if the transform ever fails, `result` was already valid JSON so it fails before the pipe — fine (same failure mode as a jq typo elsewhere). +- `.size // 0` covers entries lacking `size` (dirs carry size 0; all observed files carry real size incl. LFS files, whose top-level `size` is the true byte size). +- Do NOT change the fallback (lines 210-211) — it already emits `{rfilename, size}` (had the same shape requirement; verified live). +- Edge: tree returning a non-array (code-200 error object) — `[.[] | select… | {…}]` yields `[{rfilename:null}]`-style or `[]`; `--gguf` guard + Step-3 messages convert that to a graceful error. Acceptable; no extra guard required. + +**Change 2 — defense-in-depth guard in the `--gguf` filter (line 336).** Recommended, type-check form (strictly safe even if `rfilename` were a non-string non-null): +```bash + filtered_files="$(printf '%s' "$files_json" | jq -c '[.[] | select((.rfilename | type) == "string" and (.rfilename | endswith(".gguf")))]')" +``` +Equivalent accepted: `select(((.rfilename // "") | endswith(".gguf")))}` — both are purely defensive here (normalized data always strings); must NOT become a replacement for Change-2-Normalization: with normalization, guard-or-not both select the 10 gguf. Verified equivalent on fixture: guarded 10, unguarded 10. + +**Change 3 — mode-aware "no files" message (replaces line 344, `[ "$file_count" -gt 0 ] || err "No files to download"`).** Keep exit-1 semantics, distinct messages per mode: +```bash + local file_count + file_count="$(printf '%s' "$filtered_files" | jq 'length')" + if [ "$file_count" -eq 0 ]; then + if [ -n "$filename" ]; then + err "File not found: $filename in $repo_id (branch: ${branch})" + elif [ "$GGUF_ONLY" -eq 1 ]; then + err "No .gguf files found in $repo_id${branch:+ (branch: $branch)} — try without --gguf" + else + err "No files to download" + fi + fi +``` +Not required for the crash fix; required by edge-case spec (no-gguf repo → graceful, distinct message, not crash), and fixes the misleading "No files to download" in single-file mode. + +**Line content checks after Changes 1-3 (verified by fixture/live):** +- single-file line 333: fits; `select(.rfilename == $fn)` on normalized → 1 for exact `README.md` / `qwen2.5-3b-instruct-q4_k_m.gguf`. +- all-files line 339: fits; loop line 372 pulls real rfilename; size line 373 real; meta line 398 real list; summary 412 real. +- **Lines 372/373/376/377/398/412 need NO change** once normalized (checked on fixture). + +## 5. Edge cases — [DONE] + +| Case | Behavior before fix | After fix | +|---|---|---| +| Repo w/ only `type:"directory"` (tree) | null → crash/downstream; e.g. gguf mode crashes, all-files nulls | `select(.type=="file")` → `[]` → mode-aware graceful error | +| Empty array / empty siblings tree | crash / silent | `[]` → graceful error | +| `--gguf` on repo w/o .gguf | crash | `err "No .gguf files found in …"` (exit 1, no crash) | +| File entry missing `size` / size:0 | `.size // 0` everywhere → ok | unchanged; normalization also `// 0` | +| LFS files (`.gguf` 2GB+) | n/a (never reached) | sizes real (`2104932768`); disk pre-check works | +| Repo w/ subdirs (non-recursive tree) | null loop | dirs filtered; subtree files not listed — pre-existing semantics (flag in Step 2, decision boundary for follow-up only) | +| `--branch` non-main | field absent regardless | branch is only URL+tree-parameter; normalized same way | + +## 6. Test plan (harness spec for Builder) — [DONE] + +**Location:** `/tmp/opencode/hf-test2/` (workspace must NOT gain test files; repo has no harness for pos-ai-hf; user requirement = new fixture-based harness). + +**Key constraint — DO NOT `source` bin/pos-ai-hf in the harness**: the tool executes flag parsing + `usage`/`cmd_download` at top level (`set -euo pipefail`; no-args → `usage` → `exit 0` is a NAK). The harness must test the **jq transforms in isolation** (option b of the brief). Extraction of the functions via `sed -n` is __not__ recommended (fragile); fix the documented transforms — the transforms ARE the bug. + +**Fixture files (create in harness setup, static content):** +- `fixtures/tree-files.json` — copy of `/tmp/opencode/qwen-tree.json` (13 files, rfilename ABSENT; 10 .gguf). +- `fixtures/tree-with-dirs.json` — copy of `/tmp/opencode/sd-tree.json` (8 files + 7 `type:"directory"`). +- `fixtures/tree-empty.json` — `[]`. +- `fixtures/tree-nogguf.json` — e.g. `[{"type":"file","path":"README.md","size":100}]`. +- Set at top: `NORM='[.[] | select(.type == "file") | {rfilename: .path, size: (.size // 0)}]'` and `GGUF_FILTER='[.[] | select((.rfilename | type) == "string" and (.rfilename | endswith(".gguf")))]' FN_FILTER='[.[] | select(.rfilename == $fn)]'` — **duplicated strings; if either diverges from the file, tests catch drift when `grep -F` checks below run.** + +**Assertions (each a small `t_` function; count PASS/FAIL; exit non-zero on any fail):** +1. `t_tree_normalize`: `jq -c "$NORM" tree-001.json` → length 13; every `.size` is number; no entry has `rfilename == null`. +2. `t_gguf_on_normalized`: pipe NORM(tree-001) → GGUF_FILTER → length 10; contains `qwen2.5-3b-instruct-q4_k_m.gguf`; NOT contains `README.md`/`LICENSE`/`.gitattributes`. +3. `t_single_file`: `jq -c --arg fn "qwen2.5-3b-instruct-q4_k_m.gguf" "$FN_FILTER"` on NORM(tree-001) → length 1; on `--arg fn "no-such-file"` → 0 (no crash). +4. `t_all_files_passthrough`: `jq -c '.'` → 13; loop pipe `.[]` → 13 rows, each with string rfilename (no `null`). +5. `t_empty`: NORM on tree-empty.json → `[]` | run the mode-0 guard → error message path (grep the code); i.e. assert `printf '[]' | jq "$NORM"` outputs `[]` and file_count logic (replicate `[ "$(…|jq 'length')" -eq 0 ]`) succeeds. +6. `t_nogguf`: NORM(tree-nogguf.json) → GGUF_FILTER → length 0, no crash; assert the code contains the "No .gguf files" branch (grep). +7. `t_defense_guard`: pipe **raw** qwen-tree.json (unnormalized) through GGUF_FILTER → length 0, exit 0 (proves guard null-proof and non-weakening: the sole difference 10→0 is caused by normalization, guard itself no-op). +8. `t_dirs_excluded`: NORM(tree-with-dirs.json) → length 8 (files only; 7 dirs dropped). +9. `t_code_sync` (drift check): `grep -Fq 'select(.type == "file")' /bin/pos-ai-hf` and `grep -Fq 'endswith(".gguf")'` present — catches D it if Builder changed jq inline, test stays honest. + +**Live smoke (optional; fast, no download):** `curl` tree for Qwen → NORM → GGUF_FILTER → assert 10 rfilenames, 0 README. (This exact pipe was executed in Step 1; pass.) + +## 7. Verification commands for Builder (after implementing) + +- `bash -n bin/pos-ai-hf` (repo copy — the installed /usr/local/bin copy is byte-identical; contract must be fixed in the repo copy). +- `bash /tmp/opencode/hf-test2/run-tests.sh` → all PASS. +- `make gen && make check && make lint` → doc tables/registry unchanged; expect green, `0 FAIL, 0 WARN`, and `git diff` limited to `bin/pos-ai-hf` (+ any doc touch required by CONVENTION pointers; no GEN:START/END blocks change). +- Live no-download validation (already demonstrated passing): +```bash +curl -sS "https://huggingface.co/api/models/Qwen/Qwen2.5-3B-Instruct-GGUF/tree/main" \ + | jq '[.[] | select(.type=="file") | {rfilename:.path,size:(.size//0)}]' \ + | jq '[.[] | select(.rfilename|endswith(".gguf"))]' | jq 'length' # expect 10, no README/LICENSE +``` +- Optional tiny-download proof: `pos ai hf download Qwen/Qwen2.5-3B-Instruct-GGUF LICENSE --output /tmp/hf-small` → expect `.hf-meta` listing `LICENSE` and 1-file summary with real size; deletes nothing else. +- Full ~2GB download verified: OUT OF SCOPE (explicit). + +## Handoff + +Status: **ROOT_CAUSE_ESTABLISHED** +- Symptom: crash `jq: endswith() requires string inputs` (line 336) on --gguf; silent no-match single-file; `null` URLs/meta/summary in all-files. +- Expected vs actual: primary tree response should look like the siblings metadata (`rfilename`-keyed) but arrives free-`.rfilename` keys; first divergence = `hf_repo_files` returns raw tree (line 201-204). +- Root cause: missing shape normalization of `/tree` response in `hf_repo_files()` — user's endswith guard insufficient (guarded --gguf would download 0 files / "No files to download"). +- Classification: FACT (crash & downstream effects reproduced on live fixture; normalization + guard live-validated elsewhere). +- Alternatives eliminated: (a) network/API failure — endpoints live 200 & JSON; (b) `rfilename` present but null — keys are absent (but `oid,path,size,type`), confirmed on fetch; (c) fallback path defect — live-verified correct shape; (d) curl/URL issue in hf_download_file — reached only if loop got a non-null name; function itself defect-free. +- Affected components: `bin/pos-ai-hf` — `hf_repo_files()` (primary branch), `cmd_download()` lines 333/336/341-344 (message branch), and downstream read sites 372/398/412 (no change needed once normalized). +- Recommended next agent: **Builder** — fix is exactly scoped: one transform in `hf_repo_files()`, one defense-in-depth guard line, one message branch; implement per spec Step 4 and run Step 7 verification. **Tester** (after Builder) — no committed harness; new /tmp/opencode/hf-test2 harness + optional repeat live tests; recommend adding to repo test suite if project adopts (decision boundary: Architect). +- Remaining uncertainty: none material on the bug; only flagged out-of-scope items (hf_api `-L`/307 for alias repos; non-recursive tree semantics) for maintainers. +- Changes made by Detective: none (read-only). \ No newline at end of file diff --git a/AgentsReport/reviewer/2026-09-04_pos-ai-hf-gguf-jq-bug-review.md b/AgentsReport/reviewer/2026-09-04_pos-ai-hf-gguf-jq-bug-review.md new file mode 100644 index 0000000..8c6afd6 --- /dev/null +++ b/AgentsReport/reviewer/2026-09-04_pos-ai-hf-gguf-jq-bug-review.md @@ -0,0 +1,118 @@ +# Review Report — `pos-ai-hf` GGUF/jq bug fix (2026-09-04) + +## TL;DR +- **Verdict:** APPROVE_WITH_NOTES — no BLOCKING or REQUIRED findings. The 3-hunk fix matches the Detective spec exactly; diff is scoped; doc diff is a clean single gen line-count change; harness design is sound; user acceptance evidence confirms the real flow. +- **Findings:** 1 SUGGESTED (harness lacks an error-object/non-array shape test for the very adversarial case this review probed), plus NOTES on AGENT_TODO.md and harness message-checks being code-presence greps. +- **UNVERIFIED (sandbox):** cannot execute bash beyond read-only git/grep — harness 9/9 and `bash -n`/`make check`/`make lint` claims NOT re-run by me; Orchestrator must run them (prior-review convention). +- **Diff verdict:** PASS (3 sanctioned hunks, byte-for-byte per spec; doc = gen row only 495→506). +- **Harness verdict:** PASS on design/structure/fixture integrity; execution UNVERIFIED here. + +## Step 1: Inputs & scope — [DONE] +- Spec of record: `AgentsReport/detective/2026-09-04_pos-ai-hf-gguf-jq-bug.md` (Changes 1–3 at Step 4, lines 80–126). +- Builder report: `AgentsReport/builder/2026-09-04_pos-ai-hf-gguf-jq-bug-fix.md`. +- Diff (`git diff bin/pos-ai-hf`): exactly 3 hunks: + - `bin/pos-ai-hf:201-207` — normalization + comments in `hf_repo_files` primary path. + - `bin/pos-ai-hf:339` — guarded `--gguf` filter. + - `bin/pos-ai-hf:346-355` — mode-aware empty-message branch, replaces the old `-gt 0 || err` one-liner. +- Doc diff (`git diff DOC/AGENT_Context_Project.md`): single line, `bin/pos-ai-hf` filetable row 495→506 (`:613`); row sits inside `GEN:START filetable` (612)–`GEN:END` (659) → legitimate `make gen` output; 506 matches actual file length (read: file ends at line 506). No other DOC/GEN changes. Full file integrity further confirmed via `git diff --stat`: only `bin/pos-ai-hf` (15 ins/3 del) + doc (1/1) modified; no other working-tree files. +- POS headers (lines 1–12) untouched; exec bit `100755` (git ls-files -s). + +**Step 1 verdict:** [PASS] + +## 2: Change 1 — normalization in hf_repo_files — [DONE] +`bin/pos-ai-hf:205`: +`printf '%s' "$result" | jq '[.[] | select(.type == "file") | {rfilename: .path, size: (.size // 0)}]'` +- Literally matches spec line 97 (including comment lines 202-204). Dirs dropped via `select(.type == "file")`; `size` defaulted via `.size // 0`; output shape `{rfilename, size}` per entry. +- Adversarial probe — non-array (HTTP 200 error object `{"error":"x"}`): reasoned (cannot run jq): `.[]` on an object iterates its **values**; each value (string/object) fails `select(.type == "file")` → result `[]`, no jq error → file_count 0 → mode-aware `err` (exit 1). Only a top-level **number/boolean** 200-body would make `.[]` error ("Cannot iterate over number"); the HF tree endpoint never returns that, and `hf_api` (`:172-183`) errs on non-200 and validates JSON. This is exactly the residual accepted in spec line 104 ("graceful error"). No crash path for realistic inputs. +- `set -euo pipefail` interaction: pipe failure would abort the script (spec line 101 explicitly accepts this) — an impossible failure here since `result` passed `jq empty` validation in `hf_api` (`:181`). +- Verified on fixture data (counts, read-only): `fixtures/tree-files.json` = 13 `type:"file"` / 0 dirs / 10 `.gguf` entries; `fixtures/tree-with-dirs.json` = 8 files + 7 dirs — matches spec claims; dirs have `size: 0`, files carry real sizes (incl. LFS 3.98 GB fp16). Schema keys `oid/path/size/type`, no `rfilename` — confirms the bug's premise in the fixture. + +**Step 2 verdict**: [PASS] (error-object behavior: STRONG INFERENCE from jq semantics; empirical jq run UNVERIFIED — Orchestrator to run `printf '%s' '{"error":"x"}' | jq '[.[] | select(.type == "file") | {rfilename: .path, size: (.size // 0)}]'` — expected `[]`, rc 0.) + +## 3: Change 2 — guarded --gguf filter — [DONE] +`bin/pos-ai-hf:339` matches spec line 108 verbatim. Non-weakening — FACT by construction: `(type)=="string" and endswith(...)` is true-passthrough for every valid-domain input the old `endswith` accepted (strings), and converts the former crash (null/number) into a no-match. With normalized data both guard-on and guard-off select the same 10 — guard is purely defense-in-depth, verified conceptually on the fixture (10 gguf paths present). + +**Step 3 verdict:** [PASS] + +## 4: Change 3 — mode-aware empty messages — [DONE] +`bin/pos-ai-hf:347-355` matches spec lines 115–125 exactly: +- single-file: `err "File not found: $filename in $repo_id (branch: ${branch})"` — `$filename` only referenced under `-n "$filename"` guard (line 348), no unset risk (`local filename="${SUBCMD_ARGS[1]:-}"` at line 324 keeps it set/empty). Em-dash/text per spec. +- `--gguf`: `err "No .gguf files found in $repo_id${branch:+ (branch: $branch)} — try without --gguf"` — does NOT reference `$filename` (safe in --gguf mode); `${branch:+...}` defensive on empty branch. +- generic: `err "No files to download"` (unchanged). +- Exit semantics preserved: `err` in `lib/common.sh:24` → `exit 1` (verified read-only). No double-printing; single `err` call per branch. + +**Step 4 verdict:** [PASS] + +## 5: Regression surface — [DONE] +- `git diff` shows hunks only at 201-207, 339, 346-356 — search/list/remove, `hf_search` (216-223), `hf_api` (134-186), `hf_resolve_branch` (244-264), `hf_download_file` (267-289), URL building (387), `--branch` (326), `--output` (114-116) — all byte-identical. +- Only consumer of `files_json`/`hf_repo_files`: `cmd_download` line 330 (`grep hf_repo_files` → definition :188, call :330 — no other caller repo-wide). +- Downstream read sites now receive `{rfilename,size}`: loop (`:383-387`), size `// 0` (`:384`, `:364), meta `[.[]|.rfilename]` (`:409`), summary `.[0].rfilename` (`:423,:425`) — all fit the normalized shape; no downstream edit needed. Shape parity with fallback (`:213` `[.siblings[] | {rfilename, size:(.size//0))}]`) — identical keys `{rfilename, size}`, both numeric sizes. Fallback sizes are 0 (HF metadata API has no sizes — detective Step 1 verified); that's a data, not a shape, difference. +- No `INTERACTIVE_CMDS` impact (pos-ai-hf doesn't read stdin; not in the stdin family). + +**Step 5 verdict:** [PASS] + +## 6: Harness quality — [DONE] +`/tmp/opencode/hf-test2/run-tests.sh` (read in full; counting/verification of fixtures via wc/grep — I could not *execute* anything, sandbox policy): +- 9 assertions (lines 132-140), all behavioral jq-on-fixture checks except the intent-documented message-branch greps (t_empty:83-95, t_nogguf:98-105 — code-presence because sourcing the tool is a NAK per spec line 149; replicated `file_count` logic; design accepted). +- Not tautological: each asserts a numeric/string result (13/0-nulls sizes, 10 gguf, 1/0 single-file, 13 rows no nulls, `[]`, count 0 rc 0 raw-guard, 8 files 0-`/` paths). +- Drift-guards t_code_sync (124-129) pin all four jq expressions with `grep -F` — they pin presence, not location; divergence from the file breaks the test. Adequate per spec. +- t_defense_guard (108-112) correctly demonstrates non-weakening AND null-proofing on the raw unnormalized fixture (`exit 0`, `[]`). +- Fixture integrity: verified on-disk (13/0/10 and 8+7; `[]`; `[{"type":"file","path":"README.md","size":100}]`). +- **Gap:** no assertion for the adversarial non-array shape (`{"error": "x"}` → normalize → `[]` rc 0, and `--gguf` guard on it → `[]`); also no static fixture for it. Recommend adding (a good SUGGESTED). +- Note: `REPO` hard-coded to the home checkout path (line 10) — fine in place, would need param if the harness is ever committed for CI (out of today's decision boundary). + +**Step 6 verdict:** [PASS] (execution UNVERIFIED; design/FACT-checks passed) + +## 7: Style / conventions — [DONE] +- `set -euo pipefail` line 2 intact; `# POS:`/POS_FLAGS/DEPS/CONFIG/EXAMPLES headers lines 3-12 unchanged; exec bit 100755 (ls-files -s, pre-commit gate intact by chmod). +- No new dependencies; no unrelated files (add/modify status = only the two expected paths + 2 report artifacts). +- Doc-sync: single gen line-count row, in-GEN-block, actual-sync (506 = line count) — no hand-edit violation. +- **`AGENT_TODO.md` not touched — no task entry was created/moved for this multi-agent task; AGENTS.md asks to move finished tasks to Done. Minor; the Orchestrator can fold a dated Done line into the fix commit. (NOTE)** +- Not runnable here (bash restricted): `bash-nn`, `make gen/check/lint` claimed green by Builder (0 FAIL, 0 WARN) — UNVERIFIED; prior-review convention: Orchestrator runs these. + +**Step 7 verdict:** [PASS] with 2 NOTES + +## 8: Panic-check the user-visible flows — [DONE] (read-verified; user evidence) +| Flow | Expected after fix | Evidence | +|---|---|---| +| Single-file (`README.md`) | `1` match, real URL | fixture: `select(.rfilename == $fn)` matches a real `path` after NORM; builder live-verified `LICENSE` (7.2 KB, `.hf-meta` correct) | +| All-files | 13 rows, string rfilename | normalized fixture row count 13, 0 nulls in harness; loop `:381-402` safe | +| `--gguf` with gguf repos | 10 files, `[1/10]` progress | user acceptance: `pos ai hf download Qwen/Qwen2.5-3B-Instruct-GGUF --gguf --output ~/.models` → `[1/10] Downloading qwen2.5-3b-instruct-fp16-00001-of-00002.gguf…` (no jq crash) — matches `file_count=10` progress format (`:390-392`) | +| `--gguf` no gguf | `err "No .gguf files found in … — try without --gguf"`, exit 1 | code `:351`; Builder live-verified distilbert | +| Empty repo `[]` | `err "No files to download"` | `:353`; harness t_empty | +| Dirs-only | filtered → `[]` → graceful | NORM `select(.type=="file")`; t_dirs_excluded | + +**Step 8 verdict:** [PASS] + +## 9: Findings (numbered) +1. **SUGGESTED** — Harness lacks a non-array / error-object shape test (the adversarial probe Section 2). Add a static fixture, e.g. `{"error":"unauthorized"}` (and optionally `null`), with assertions: NORM → `[]`, rc 0; GGUF_FILTER on it → `[]`, rc 0 — the harness would then also pin the graceful-shape property it currently only watches through NORM. Evidence: `run-tests.sh` contains no such case. `Relevant file: /tmp/opencode/hf-test2/run-tests.sh` (lines 36-129). Approved reference: Detective Step 5 ("Tree returning a non-array (code-200 error object) … acceptable; no extra guard"), which the harness should lock in. Why it matters: this review's adversarial probe could only be **reasoned** (cannot run jq), and it is the one untested branch of the new code; low cost to pin. +- **NOTE** — `AGENT_TODO.md` has no entry for this task (head shows empty Now); per AGENTS.md convention a Done-line should be folded into the fix commit. Not defect-scope (the fix commit doesn't exist yet); Orchestrator to fold in at commit. +- **NOTE** — the follow-up `AGENT_TODO.md` / message-branch tests in the harness are grep-presence checks rather than full command runs by design (sourcing is top-level-NAK); behavioral side is covered by Builder+user live evidence. +- **UNVERIFIED** — harness 9/9, `bash -n`, `make gen/check/lint` (green) — this sandbox denies non-git bash execution; couldn't re-run. Builder's claims are internally consistent with the diff and fixtures; user's live run independently corroborates the core fix. Orchestrator runs: `bash /tmp/opencode/hf-test2/run-tests.sh`, `bash -n bin/pos-ai-hf`, `make gen && make check && make lint`, and the error-object jq one-liner from Step 2. + +## Verification verified +- Statements contract: Change 1/2/3 match spec line-for-line (verified to file content). +- Shape parity primary↔fallback (identical `{rfilename, size}` keys). +- Scope containment: 15/3 lines in `bin/pos-ai-hf` + 1 DOC row; 3 hunks; no out-of-scope code. +- Exec bit, headers, deps, convention surface unchanged. +- Fixture integrity on disk: 13 (0 dirs, 10 gguf) + 8 files/7 dirs + `[]` + no-gguf; schema proofs the bug premise. +- Exit-1 semantics through `err` (`lib/common.sh:24`). + +## Verification unverified +- jq behavior on `{"error":"x"}` (reasoned only: `[]`, rc 0). +- Harness 9/9 run + `make check` and `make lint` (needs Orchestrator). +- Installed `/usr/local/bin` copy byte-identity (user-run success implies fixed code; path check not assessable here). + +## Scope compliance +- In-scope: the 3 Changes 1-3 from Detective Step 4 + harness per Step 6. **All present, nothing extra.** +- Out-of-scope found: none (docs/out-of-scope flags — `hf_api` NO `-L` 307 alias handling, non-recursive tree — correctly not touched, deferred following spec Step 2). + +## Remaining uncertainty +- Empirical jq semantics on the error-object case judged safe but not executed by me (read-only boundary); one-liner for Orchestrator. +- Orchestrator-significant matters: harness/make execution results as acceptance evidence. + +## Recommended next agent +**Orchestrator** — approve-and-commit: stage `bin/pos-ai-hf` + `DOC/AGENT_Context_Project.md` + the two reports; fold a dated AGENT_TODO Done line per convention (NOTE 2); run the 3 harness/gates; optionally attach the error-object jq one-liner (Step 2) to close uncertainty. If any gate candidate genuinely fails, return to Builder within this exact scope (defects would be mechanical, not design). + +## Changes made by Reviewer +none (read-only; no repo file modified; only this report written) \ No newline at end of file diff --git a/DOC/AGENT_Context_Project.md b/DOC/AGENT_Context_Project.md index 5ac2d24..23b3b7d 100644 --- a/DOC/AGENT_Context_Project.md +++ b/DOC/AGENT_Context_Project.md @@ -613,7 +613,7 @@ Use conventional prefixes: `feat:`, `fix:`, `docs:`, `refactor:`, `chore:` | `bin/pos` | 302 | CLI dispatcher with smart arg matching + logging + category help | | `bin/pos-ai-alias` | 760 | manage AI agent aliases | | `bin/pos-ai-gemini` | 7 | Forward to pos ai --provider gemini (backward compat) | -| `bin/pos-ai-hf` | 495 | Download AI models from Hugging Face (search, download, manage) | +| `bin/pos-ai-hf` | 506 | Download AI models from Hugging Face (search, download, manage) | | `bin/pos-ai-openrouter` | 7 | Forward to pos ai --provider openrouter (backward compat) | | `bin/pos-ai-server` | 444 | llama.cpp local inference server (start, stop, status, models, logs) | | `bin/pos-communication-matrix-listener` | 568 | Matrix listener: map /command → bash, run them on room messages | diff --git a/bin/pos-ai-hf b/bin/pos-ai-hf index eb41712..114a500 100755 --- a/bin/pos-ai-hf +++ b/bin/pos-ai-hf @@ -199,7 +199,10 @@ hf_repo_files() { local endpoint="/models/${ns}/${repo}/tree/${branch}" local result if result="$(hf_api "$endpoint" 2>/dev/null)"; then - printf '%s' "$result" + # Tree API returns {type,path,size,oid[,lfs]} per entry — normalize to the + # {rfilename,size} shape the rest of the pipeline expects (same as fallback). + # Skip "directory" entries and guard non-object entries (error objects crash .[]). + printf '%s' "$result" | jq '[.[] | select(type == "object" and .type == "file") | {rfilename: .path, size: (.size // 0)}]' return 0 fi @@ -207,7 +210,7 @@ hf_repo_files() { warn "Tree endpoint unavailable, using repo metadata" local fallback fallback="$(hf_api "/models/${ns}/${repo}")" || err "Failed to fetch repo info for $repo_id" - printf '%s' "$fallback" | jq '[.siblings[] | {rfilename: .rfilename, size: (.size // 0)}]' + printf '%s' "$fallback" | jq '[.siblings[]? | select(type == "object") | {rfilename: (.rfilename // ""), size: (.size // 0)}]' } hf_search() { @@ -333,7 +336,7 @@ cmd_download() { filtered_files="$(printf '%s' "$files_json" | jq -c --arg fn "$filename" '[.[] | select(.rfilename == $fn)]')" elif [ "$GGUF_ONLY" -eq 1 ]; then # GGUF filter - filtered_files="$(printf '%s' "$files_json" | jq -c '[.[] | select(.rfilename | endswith(".gguf"))]')" + filtered_files="$(printf '%s' "$files_json" | jq -c '[.[] | select((.rfilename | type) == "string" and (.rfilename | endswith(".gguf")))]')" else # All files filtered_files="$(printf '%s' "$files_json" | jq -c '.')" @@ -341,7 +344,15 @@ cmd_download() { local file_count file_count="$(printf '%s' "$filtered_files" | jq 'length')" - [ "$file_count" -gt 0 ] || err "No files to download" + if [ "$file_count" -eq 0 ]; then + if [ -n "$filename" ]; then + err "File not found: $filename in $repo_id (branch: ${branch})" + elif [ "$GGUF_ONLY" -eq 1 ]; then + err "No .gguf files found in $repo_id${branch:+ (branch: $branch)} — try without --gguf" + else + err "No files to download" + fi + fi # Prepare target directory local target_dir