This commit is contained in:
@@ -0,0 +1,258 @@
|
||||
# Reviewer Report — `pos ai hf` (Hugging Face Model Downloader)
|
||||
|
||||
**Date:** 2026-09-04
|
||||
**Status:** CHANGES_REQUIRED
|
||||
|
||||
---
|
||||
|
||||
## TL;DR
|
||||
|
||||
**Verdict: CHANGES_REQUIRED**
|
||||
|
||||
Reviewed: `bin/pos-ai-hf` (470 lines), `DOC/POS.md` updates, generated docs/completions, AGENT_TODO entry.
|
||||
|
||||
**1 BLOCKING finding** — `total_size` accumulated inside a pipe subshell is always 0, so multi-file download summaries show incorrect total size. **2 REQUIRED findings** — missing disk space pre-flight check (architect-specified) and missing `hf_repo_files` API fallback (architect-specified). **2 SUGGESTED findings** — rate-limit HEAD request inefficiency and error message format deviation.
|
||||
|
||||
| Severity | Count |
|
||||
|----------|-------|
|
||||
| BLOCKING | 1 |
|
||||
| REQUIRED | 2 |
|
||||
| SUGGESTED | 2 |
|
||||
| NOTE | 3 |
|
||||
|
||||
---
|
||||
|
||||
## Checklist Results
|
||||
|
||||
### Code Quality
|
||||
|
||||
| Item | Status | Evidence |
|
||||
|------|--------|----------|
|
||||
| `set -euo pipefail` present | [PASS] | Line 2: `set -euo pipefail` |
|
||||
| `# POS:` header correct format with em-dash | [PASS] | Line 3: `# POS: ai hf — Download AI models from Hugging Face (search, download, manage)` — em-dash `—` confirmed |
|
||||
| `# POS_FLAGS:` correct | [PASS] | Line 4: `# POS_FLAGS: --branch --gguf --output` — matches actual flag parsing (lines 88-109) |
|
||||
| `# POS_DEPS:` correct | [PASS] | Line 5: `# POS_DEPS: curl jq` — matches deps guards on lines 17-18 |
|
||||
| `# POS_CONFIG:` correct format | [PASS] | Line 6: `# POS_CONFIG: ai \| ai.env \| HF_TOKEN=secret:… \| HF_DOWNLOAD_DIR=:…` — uses `secret:` prefix convention matching other tools (pos-docker-compose, pos-communication-matrix-sender, pos-communication-telegram-sender) |
|
||||
| `# POS_EXAMPLES:` present | [PASS] | Lines 7-12: 6 example lines covering search, download (repo, gguf, single file), list, remove |
|
||||
| Sources `lib/common.sh` via standard fallback | [PASS] | Line 14: `source "$(dirname "$0")/../lib/common.sh" 2>/dev/null \|\| source "$(dirname "$0")/common.sh"` — exact template pattern |
|
||||
| Deps guards BEFORE `-h\|--help` | [PASS] | Lines 17-18 (deps) before line 90 (`-h\|--help` case) |
|
||||
| `usage()` present and comprehensive | [PASS] | Lines 44-79: all 4 subcommands, download options, examples, config keys, exit codes documented |
|
||||
| All 4 subcommands implemented | [PASS] | `cmd_search` (line 279), `cmd_download` (line 303), `cmd_list` (line 402), `cmd_remove` (line 438); dispatch at line 464 |
|
||||
| Config loader reads `ai.env` with env-var precedence | [PASS] | `load_hf_config()` (lines 25-39): reads `CONFIG_FILE`, env-already-set wins (`if [ -z "${!k:-}" ]`), strips quotes, CR, comments |
|
||||
| Auth header: `Authorization: Bearer $HF_TOKEN` | [PASS] | `hf_auth_header()` (lines 128-132): `printf 'Authorization: Bearer %s' "$HF_TOKEN"` |
|
||||
| Rate limit handling: 429 → sleep + retry | [PASS] | Lines 150-167: loop with `attempt < 2`, on 429 extracts `Retry-After` or defaults 60, sleeps, retries once |
|
||||
| Resume: `curl -C -` | [PASS] | Line 259: `curl_args=(-L -C - --progress-bar -o "$target")` |
|
||||
| `.hf-meta` metadata tracking | [PASS] | Lines 368-382: writes JSON with repo_id, branch, timestamp, files array |
|
||||
| Output matches contract (emojis, paths, sizes) | [PASS] | Lines 392-398: 📥 and 📁 emojis, repo-id, size, path format matches Architect Decision 6 |
|
||||
| Error handling: 404 | [PASS] | Line 175: `err "Model not found: ${endpoint#/api/models/}"` |
|
||||
| Error handling: 401/403 | [PASS] | Line 174: `err "Authentication failed — check HF_TOKEN (pos config ai)"` |
|
||||
| Error handling: 429 | [PASS] | Line 176: `err "Rate limit exceeded — try again later"` |
|
||||
| Error handling: timeout | [PASS] | Line 153: `err "Connection timed out — check network"` |
|
||||
| Error handling: jq parse | [PASS] | Line 182: `err "Failed to parse API response — check network or HF status"` |
|
||||
| Error handling: no token | [PASS] | Line 121: `warn "No HF_TOKEN set — using anonymous access"` — continues for public repos per spec |
|
||||
| All file paths seam-guarded | [PASS] | `CONFIG_FILE="${CONFIG_FILE:-$HOME/…}"` (line 21), `HF_TOKEN="${HF_TOKEN:-}"` (line 22), `HF_DOWNLOAD_DIR="${HF_DOWNLOAD_DIR:-$HOME/…}"` (line 23), re-guarded at line 117 after `--output` override |
|
||||
| No `err "msg" 1` pattern | [PASS] | All 19 `err` calls use `err "message"` with no trailing exit code — confirmed by grep |
|
||||
|
||||
### Convention Compliance
|
||||
|
||||
| Item | Status | Evidence |
|
||||
|------|--------|----------|
|
||||
| `bash -n` passes | [UNVERIFIED] | Cannot execute `bash -n` due to sandbox restrictions. Builder claims pass. |
|
||||
| `make gen && make check` passes | [UNVERIFIED] | Cannot execute make. Builder claims pass. Generated files (AGENT_Context, completions) contain correct entries. |
|
||||
| `make lint` passes (0 FAIL, 0 WARN) | [UNVERIFIED] | Cannot execute make. Builder claims 0 FAIL, 0 WARN. |
|
||||
| Tool is executable (chmod 100755) | [UNVERIFIED] | Cannot check permissions. `git ls-files` shows file is tracked. Builder claims chmod 100755. |
|
||||
| POS.md has `ai hf` row + detail block | [PASS] | POS.md line 58: `bin/pos-ai-hf` in `**File:**` line. Lines 103-112: command table + auth/rate-limit/resume detail block. |
|
||||
| No INTERACTIVE_CMDS change needed | [PASS] | `bin/pos` line 269: `pos-ai-hf` NOT in `INTERACTIVE_CMDS` string — tool does not read stdin. |
|
||||
| No changes to `bin/pos-ai` | [PASS] | Grep for "hf" in `bin/pos-ai` returns 0 matches. |
|
||||
| No changes to `bin/pos` | [PASS] | `pos-ai-hf` not in `INTERACTIVE_CMDS`. No other modifications visible. |
|
||||
| No changes to `preinstall.sh` | [PASS] | Not in git diff. |
|
||||
| No changes to `lib/common.sh` | [PASS] | Not in git diff. |
|
||||
| Generated docs contain ai-hf | [PASS] | AGENT_Context: tree line 66, dispatch line 283, filetable line 613. Completions line 6: `_pos_flags[ai-hf]="--branch --gguf --output"` |
|
||||
| AGENT_TODO Done entry added | [PASS] | Git diff shows new entry at top of Done section, dated 2026-09-04. |
|
||||
|
||||
### Security
|
||||
|
||||
| Item | Status | Evidence |
|
||||
|------|--------|----------|
|
||||
| Token never printed in output | [PASS] | `$HF_TOKEN` referenced only at lines 22, 120, 129-130, 138, 257 — none in any `printf`/`echo` output path. Token warning (line 121) only prints the literal string "No HF_TOKEN set". |
|
||||
| Token passed via header, not URL | [PASS] | Lines 128-132: `hf_auth_header()` constructs `Authorization: Bearer …` header. Lines 144-146, 260-262: added as `-H` arg to curl. Never in URL string. |
|
||||
| Config file permissions (ai.env chmod 600) | [UNVERIFIED] | Tool reads from `~/.config/linux_post_install/ai.env`. The chmod is set by `postinstall.sh` (not by this tool). The tool does NOT change permissions — correct behavior. |
|
||||
| No command injection via repo-id | [PASS] | `repo_id` is user input. Used in: API endpoint construction (line 198-199, passed as curl URL arg — safe), `hf_repo_dir()` (line 214: string substitution `${repo_id//\//-}` — safe), jq filter argument (line 322: `--arg fn "$filename"` — safe), `.hf-meta` heredoc (line 375-382: unquoted heredoc — variables expanded but context is JSON file, not shell execution). No `eval`, no `exec` with user-controlled path. |
|
||||
| No command injection via filenames | [PASS] | Filenames from API response are parsed by `jq -r` and used in path construction (`$target="${target_dir}/${fname}"`). Passed to `mkdir -p` and curl `-o` — no shell interpretation of the filename value itself. |
|
||||
|
||||
---
|
||||
|
||||
## Step 1: Subshell Variable Loss in Multi-File Downloads
|
||||
|
||||
[FAIL — BLOCKING]
|
||||
|
||||
**Finding:** In `cmd_download`, the multi-file download loop at lines 345-366 runs inside a pipe (`printf … | jq … | while IFS= read -r file_json; do … done`). In bash, a pipe creates a subshell, so variables modified inside the `while` loop — specifically `$total_size` (line 349) and `$downloaded` (line 355) — are lost when the pipe exits. The summary section at line 396 reads `$total_size` which is still `0` from its initialization at line 340.
|
||||
|
||||
**Severity:** BLOCKING
|
||||
**Certainty:** FACT — provable from bash subshell semantics. `cmd | while read; do var=...; done` runs the while body in a subshell. Variables set inside do not propagate back.
|
||||
**Evidence:**
|
||||
- Line 340: `local total_size=0`
|
||||
- Line 345: `printf … | jq … | while IFS= read -r file_json; do` — pipe creates subshell
|
||||
- Line 349: `total_size=$((total_size + fsize))` — modified inside subshell, lost
|
||||
- Line 396: `total_human="$(hf_human_size "$total_size")"` — reads `0`
|
||||
**Relevant files/lines:** `bin/pos-ai-hf:340-398`
|
||||
**Approved scope reference:** Architect Decision 6 (Output Contract) specifies multi-file summary as "📥 Downloaded: meta-llama/Llama-3.1-8B-Instruct (7 files, 4.7 GB)" — the size should be the correct total.
|
||||
**Why it matters:** Every multi-file download (the common case for large models) will print "0 B" as the total size. This is a user-visible incorrect output and directly violates the Architect's output contract.
|
||||
**Test mask:** The test harness (Test 22, line 151) checks for `[0-9] B)` which matches "0 B)" — the test passes on the bug. The test needs to check the actual expected sum (618 + 8500000000 + 9000000 + 5000 + 200 + 500 = 8509017318 bytes ≈ "8.5 GB").
|
||||
**Suggested fix:** Replace the pipe with process substitution (`while IFS= read -r file_json; do … done < <(printf '%s' "$filtered_files" | jq -c '.[]')`) to keep the loop in the main shell, or accumulate total_size via a temp file or another jq pass on `$filtered_files` before the loop.
|
||||
|
||||
---
|
||||
|
||||
## Step 2: Missing Disk Space Pre-Flight Check
|
||||
|
||||
[FAIL — REQUIRED]
|
||||
|
||||
**Finding:** The architecture (Decision 5, "Key implementation details" table, and Decision 7, "Error matrix" row) specifies a pre-flight disk space check: `df` available space vs estimated total (from `/tree/` endpoint), with a warning when space is critically low. The implementation has no disk space check at all.
|
||||
|
||||
**Severity:** REQUIRED
|
||||
**Certainty:** FACT — no `df` or `stat`-based space check anywhere in the file (confirmed by grep).
|
||||
**Relevant files/lines:** `bin/pos-ai-hf` — absent between line 333 (file count check) and line 337 (mkdir).
|
||||
**Approved scope reference:** Architect Decision 5: "Disk space | Pre-flight check: `df` available space vs estimated total (from `/tree/` endpoint)" and Decision 7: "Disk space | `df` pre-flight | `warn "Low disk space: need {N} GB, only {M} GB available"` then continue (user's call)".
|
||||
**Why it matters:** Downloading a 7-8 GB model on a near-full disk is a waste of time and leaves partial files. The architecture explicitly chose a non-blocking warning (not an error) — the user makes the final call. Without this, users discover the problem only after curl fails mid-file.
|
||||
**Suggested fix:** Before the download loop, sum the sizes from `$filtered_files` (this also solves the subshell bug if using jq for the sum), compare with `df --output=avail "$target_dir"`, and `warn` if insufficient. This is a ~5 line addition.
|
||||
|
||||
---
|
||||
|
||||
## Step 3: Missing `hf_repo_files` API Fallback
|
||||
|
||||
[FAIL — REQUIRED]
|
||||
|
||||
**Finding:** The architecture specifies `hf_repo_files()` should call `/api/models/{ns}/{repo}/tree/{branch}/` for file sizes, and fall back to `/api/models/{ns}/{repo}` for file list if the tree endpoint fails. The implementation only calls the tree endpoint (line 198) with no fallback.
|
||||
|
||||
**Severity:** REQUIRED
|
||||
**Certainty:** FACT — line 198: `local endpoint="/models/${ns}/${repo}/tree/${branch}"` followed by a single `hf_api "$endpoint"` call. No fallback logic.
|
||||
**Relevant files/lines:** `bin/pos-ai-hf:188-200`
|
||||
**Approved scope reference:** Architect Decision 5 (`hf_repo_files()` signature): "Calls: GET /api/models/{ns}/{repo}/tree/{branch}/ for sizes, **falls back to /api/models/{ns}/{repo} for file list**"
|
||||
**Why it matters:** Some HF repositories (e.g., datasets, some model repos) may not respond to the `/tree/` endpoint (404 or empty). The fallback to `/api/models/{ns}/{repo}` provides a file list (without sizes) so the user can still download. Without it, those repos fail entirely with a 404 error.
|
||||
**Suggested fix:** Wrap the tree call in a conditional; on 404, call `/api/models/{ns}/{repo}` and construct a minimal `[{"rfilename": "<name>", "size": 0}]` array from the `siblings` array. `size` being 0 is acceptable (displays as "0 B") since the primary goal is getting the download list.
|
||||
|
||||
---
|
||||
|
||||
## Step 4: Rate-Limit Extra HEAD Request
|
||||
|
||||
[NOTE — SUGGESTED]
|
||||
|
||||
**Finding:** Line 158 makes a second `curl -sI` HEAD request specifically to extract the `Retry-After` header value after receiving a 429. This is an extra HTTP call that could itself be rate-limited, and the `Retry-After` header was already present in the original request's response (line 151 uses `-w '%{http_code}'` but does not capture response headers).
|
||||
|
||||
**Severity:** SUGGESTED
|
||||
**Certainty:** FACT — line 158: `retry_after="$(curl -sI -H "${auth_header:-}" "$url" 2>/dev/null | grep -i 'retry-after:' | tr -d '\r' | awk '{print $2}')"`
|
||||
**Relevant files/lines:** `bin/pos-ai-hf:156-161`
|
||||
**Approved scope reference:** Architect Decision 5: "Rate limiting | Sleep 1s between files; on 429, wait `Retry-After` header value or 60s default"
|
||||
**Why it matters:** Minor inefficiency. When already rate-limited, making another request is suboptimal. Could use `curl -sS -D -` (dump headers to stdout) in the original request to capture `Retry-After` directly, or simply default to 60s without the extra call.
|
||||
**Suggested fix:** Change the original curl in `hf_api()` to use `-D -` (or a header dump file) so the `Retry-After` header is available from the first response without a second call.
|
||||
|
||||
---
|
||||
|
||||
## Step 5: Error Message Format Deviation
|
||||
|
||||
[NOTE — SUGGESTED]
|
||||
|
||||
**Finding:** The error message for invalid repo format at line 195 (`hf_repo_files`) says `"Invalid repo format: use namespace/model-name"` while the Architect specified `"Invalid repo format: use namespace/model-name"` at Decision 7. However, line 308 (`cmd_download`) also says the same message. The Architect's error matrix entry says `err "Invalid repo format: use namespace/model-name"` — which matches. This is consistent.
|
||||
|
||||
However, the Architect's error matrix says the model-not-found message should reference the full `repo-id` (e.g., `err "Model not found: {repo-id}"`), while the implementation at line 175 constructs the message from the API endpoint: `err "Model not found: ${endpoint#/api/models/}"`. The stripped endpoint value is the same as repo-id when the endpoint is `/models/{ns}/{repo}`, but if the endpoint is `/models/{ns}/{repo}/tree/{branch}`, the stripped value would be `{ns}/{repo}/tree/{branch}` — which is confusing.
|
||||
|
||||
**Severity:** SUGGESTED
|
||||
**Certainty:** HYPOTHESIS — only manifests when 404 is returned from `/tree/{branch}` endpoint (which strips to `{ns}/{repo}/tree/{branch}` in the message). The normal `/models/{ns}/{repo}` path produces the correct repo-id in the message.
|
||||
**Relevant files/lines:** `bin/pos-ai-hf:175`
|
||||
**Why it matters:** Minor UX: a 404 from the tree endpoint would show a confusing path in the error message instead of the clean repo-id. The Architect's spec just says `{repo-id}`.
|
||||
**Suggested fix:** Capture the `repo_id` and pass it to `hf_api` or handle the error at the caller level where `repo_id` is available. Or, in `hf_api`, accept an optional display-name parameter for error messages.
|
||||
|
||||
---
|
||||
|
||||
## Architect Compliance
|
||||
|
||||
| Decision | Implemented? | Notes |
|
||||
|----------|-------------|-------|
|
||||
| Decision 1: File location `bin/pos-ai-hf` | [YES] | Created at correct path |
|
||||
| Decision 2: Subcommands (download, search, list, remove) | [YES] | All 4 implemented |
|
||||
| Decision 3: Config scope extends `ai` | [YES] | `# POS_CONFIG: ai \| ai.env` — correct |
|
||||
| Decision 4: Download directory layout | [YES] | `<namespace>-<model-name>/` under XDG data dir, `.hf-meta` metadata |
|
||||
| Decision 5: Download logic | [PARTIAL] | Download flow correct; missing disk space check; missing API fallback |
|
||||
| Decision 6: Output contract | [PARTIAL] | Emojis, paths correct; multi-file size is always 0 (subshell bug) |
|
||||
| Decision 7: Error handling | [PARTIAL] | All error matrix cases handled; missing disk space pre-flight |
|
||||
| Decision 8: Deps/lint compliance | [YES] | POS headers correct, deps before help, source chain |
|
||||
| Decision 9: Testing strategy | [YES] | Stub-PATH harness exists at expected location with 46 tests |
|
||||
|
||||
**Deviations from Architect design:**
|
||||
1. Disk space pre-flight check not implemented (architect-specified, REQUIRED).
|
||||
2. `hf_repo_files` API fallback not implemented (architect-specified, REQUIRED).
|
||||
3. Rate-limit handling uses extra HEAD request instead of extracting from original response (architect did not specify implementation detail — minor deviation).
|
||||
4. Config loader is an inline pattern rather than copying from `bin/pos-ai` (functionally equivalent, not a deviation in behavior).
|
||||
|
||||
---
|
||||
|
||||
## Verification Verified
|
||||
|
||||
| Claim | Evidence | Status |
|
||||
|-------|----------|--------|
|
||||
| Builder: "46/46 tests green" | Test harness exists at `/tmp/opencode/hf-test/run-tests.sh` with 45 numbered tests visible (tests 1-45). Could not execute to confirm count. | UNVERIFIED |
|
||||
| Builder: "make gen && make check OK" | Generated files (AGENT_Context line 66/283/613, completions line 6) contain correct ai-hf entries. | STRONG INFERENCE |
|
||||
| Builder: "make lint 0 FAIL, 0 WARN" | All conventions verified by static analysis (POS headers, deps guards, source chain, exec bits claimed). | UNVERIFIED |
|
||||
| Builder: "bash -n OK" | Cannot execute. No syntax errors visible by manual inspection. | UNVERIFIED |
|
||||
| Builder: "POS.md updated" | Git diff confirms: ai File line updated (line 58), command table + detail block added (lines 103-112). | FACT |
|
||||
| Builder: "No changes to bin/pos, bin/pos-ai, preinstall.sh, lib/common.sh" | `git ls-files` confirms these files tracked; grep confirms no hf-related changes. | FACT |
|
||||
|
||||
---
|
||||
|
||||
## Verification Unverified
|
||||
|
||||
| Claim | Reason |
|
||||
|-------|--------|
|
||||
| `bash -n` passes | Sandbox prevents execution |
|
||||
| `make gen && make check` passes | Sandbox prevents execution |
|
||||
| `make lint` 0 FAIL 0 WARN | Sandbox prevents execution |
|
||||
| Tool is executable (100755) | Sandbox prevents `ls -la` |
|
||||
| Test harness 46/46 passes | Sandbox prevents execution |
|
||||
| Config file ai.env chmod 600 | Controlled by postinstall.sh, not this tool |
|
||||
|
||||
---
|
||||
|
||||
## Scope Compliance
|
||||
|
||||
**In-scope (confirmed):**
|
||||
- `bin/pos-ai-hf` created with all 4 subcommands
|
||||
- `DOC/POS.md` ai hf row + detail block
|
||||
- Generated docs/completions updated via `make gen`
|
||||
- `AGENT_TODO.md` Done entry
|
||||
|
||||
**Out-of-scope (confirmed NOT present):**
|
||||
- No ollama integration
|
||||
- No parallel downloads
|
||||
- No changes to `bin/pos-ai`, `bin/pos`, `preinstall.sh`, `lib/common.sh`
|
||||
- No new config scope (extends `ai`)
|
||||
- No `INTERACTIVE_CMDS` change
|
||||
|
||||
**Unexpected changes:** None detected.
|
||||
|
||||
---
|
||||
|
||||
## Remaining Uncertainty
|
||||
|
||||
1. Whether `bash -n`, `make gen`, `make check`, and `make lint` actually pass — Builder claims they do, and generated artifacts are consistent with this, but execution was blocked.
|
||||
2. Whether the 46 test cases actually all pass — the test harness exists with the correct structure, but Test 22 masks the subshell bug (checks for `[0-9] B)` which matches "0 B)").
|
||||
3. Whether the tool is truly `chmod 100755` — Builder claims it is, file is in git index.
|
||||
4. Live network behavior against real `huggingface.co` — only stub-tested, not end-to-end verified.
|
||||
|
||||
---
|
||||
|
||||
## Recommended Next Agent
|
||||
|
||||
**Builder**
|
||||
|
||||
**Reason:** The 1 BLOCKING finding (subshell variable loss) and 2 REQUIRED findings (disk space check, API fallback) are all within the approved scope and well-understood fixes. The Builder should:
|
||||
1. Fix the subshell bug (replace pipe with process substitution or pre-compute sum via jq)
|
||||
2. Add the disk space pre-flight check (~5 lines, `df` + `warn`)
|
||||
3. Add the `hf_repo_files` fallback to `/api/models/{ns}/{repo}` on 404
|
||||
4. Update Test 22 to check actual expected size value instead of regex `[0-9] B)`
|
||||
5. Re-run `bash -n`, tests, and gates
|
||||
|
||||
---
|
||||
|
||||
## Changes Made by Reviewer
|
||||
|
||||
None — read-only review.
|
||||
@@ -0,0 +1,426 @@
|
||||
# Independent Review: `pos ai server` — llama.cpp Inference Server
|
||||
|
||||
## TL;DR
|
||||
|
||||
**Status: ACCEPT_WITH_NOTES**
|
||||
**Verdict:** Implementation faithfully satisfies the approved architecture. All 4 case additions to pos-ai are correct; the new tool and provider adapter follow established project conventions. One REQUIRED finding (test harness outside repo scope) and several notes. No BLOCKING or CRITICAL issues.
|
||||
|
||||
**Defect count:**
|
||||
- BLOCKING: 0
|
||||
- REQUIRED: 1 (test harness artifacts at `/tmp/opencode/` — not in repo, cannot be run independently)
|
||||
- SUGGESTED: 2
|
||||
- NOTE: 4
|
||||
|
||||
---
|
||||
|
||||
## Step 1: pos-ai-server — Tool Structure & POS Headers
|
||||
|
||||
- `set -euo pipefail` present: `bin/pos-ai-server:2` — **FACT**
|
||||
- POS header correct format with em-dash: `bin/pos-ai-server:3` — `# POS: ai server — llama.cpp local inference server (start, stop, status, models, logs)` — **FACT**
|
||||
- POS_SUBCMDS: `start stop status models logs` — matches architect Decision 1:44 — **FACT**
|
||||
- POS_FLAGS: `--port --host --model --ctx --gpu --threads` — matches architect Decision 1:48 — **FACT**
|
||||
- POS_DEPS: `curl jq` — matches architect Decision 1:47 — **FACT**
|
||||
- Sources `lib/common.sh` via standard fallback chain: `bin/pos-ai-server:8` — same pattern as pos-ai-hf, pos-network-download — **FACT**
|
||||
|
||||
`[PASS]`
|
||||
|
||||
---
|
||||
|
||||
## Step 2: pos-ai-server — Deps Guards & Help
|
||||
|
||||
- Deps guards (`curl`, `jq`) at lines 11–12, BEFORE the `-h|--help` case at line 215 — correct ordering per AGENTS.md conventions — **FACT**
|
||||
- `command -v` pattern matches existing tools — **FACT**
|
||||
|
||||
`[PASS]`
|
||||
|
||||
---
|
||||
|
||||
## Step 3: pos-ai-server — All 5 Subcommands Implemented
|
||||
|
||||
- `cmd_start()` at line 258 — generates systemd unit, enables, health-checks — **FACT**
|
||||
- `cmd_stop()` at line 341 — disable, remove unit, daemon-reload — **FACT**
|
||||
- `cmd_status()` at line 356 — service state, model from `/v1/models`, config, health — **FACT**
|
||||
- `cmd_models()` at line 407 — scans `HF_DOWNLOAD_DIR` for `.gguf` files — **FACT**
|
||||
- `cmd_logs()` at line 429 — `journalctl --user -u pos-ai-server -n <lines>` — **FACT**
|
||||
- Dispatch at line 436: empty→usage, start/stop/status/models/logs→respective functions, `*`→error — **FACT**
|
||||
|
||||
`[PASS]`
|
||||
|
||||
---
|
||||
|
||||
## Step 4: pos-ai-server — Config Loader
|
||||
|
||||
- `load_config()` at line 21–35, env-var precedence via `if [ -z "${!k:-}" ]` — matches pos-ai-hf pattern (architect Decision 11:1) — **FACT**
|
||||
- Called at line 37 (top-level, before dispatch) — **FACT**
|
||||
- Pattern: reads `[A-Z_]+=` lines, strips quotes, strips CR, skips comments — identical to `bin/pos-ai:130–160` — **FACT**
|
||||
|
||||
`[PASS]`
|
||||
|
||||
---
|
||||
|
||||
## Step 5: pos-ai-server — Seam-Guarded Paths
|
||||
|
||||
- `CONFIG_FILE="${CONFIG_FILE:-$HOME/.config/linux_post_install/ai.env}"` — env-overridable — **FACT**
|
||||
- `USER_SYSTEMD_DIR="${USER_SYSTEMD_DIR:-${XDG_CONFIG_HOME:-$HOME/.config}/systemd/user}"` — env-overridable, matches pos-network-download:23 — **FACT**
|
||||
- `HF_DOWNLOAD_DIR="${HF_DOWNLOAD_DIR:-$HOME/.local/share/linux_post_install/ai/models}"` — env-overridable — **FACT**
|
||||
|
||||
`[PASS]`
|
||||
|
||||
---
|
||||
|
||||
## Step 6: pos-ai-server — Binary Detection Fallback
|
||||
|
||||
- `find_llamacpp()` at line 40–47: candidates `llama-server`, `llama.cpp/server`, `server`, `llama-server-cuda` — matches architect Decision 9:454–460 exactly — **FACT**
|
||||
- Called in `cmd_start()` with `|| err "..."` on failure — **FACT**
|
||||
|
||||
`[PASS]`
|
||||
|
||||
---
|
||||
|
||||
## Step 7: pos-ai-server — GPU Detection
|
||||
|
||||
- `detect_gpu()` at line 50–56: `nvidia-smi` check with proper stderr suppression — matches architect Decision 5 — **FACT**
|
||||
- `resolve_gpu_layers()` at line 58–71: configured→use value; `-1`→auto-detect → cuda→`-1`, cpu→`0` — matches architect Decision 5:270–285 — **FACT**
|
||||
- Warning at line 277–279: "No NVIDIA GPU detected — running in CPU mode" — matches architect Decision 9 error table — **FACT**
|
||||
|
||||
`[PASS]`
|
||||
|
||||
---
|
||||
|
||||
## Step 8: pos-ai-server — Systemd Unit Generation
|
||||
|
||||
- Generated at runtime via heredoc (`cat > "$USER_SYSTEMD_DIR/$SERVICE" <<EOF`) — matches pos-network-download:173 pattern — **FACT**
|
||||
- Unit fields:
|
||||
- `Type=simple` — matches architect Decision 2:75 — **FACT**
|
||||
- `Restart=on-failure` — matches architect Decision 2:76 — **FACT**
|
||||
- `RestartSec=5` — matches architect Decision 2:77 — **FACT**
|
||||
- `TimeoutStopSec=10` — matches architect Decision 2:78 — **FACT**
|
||||
- `KillMode=control-group` — matches architect Decision 2:79 — **FACT**
|
||||
- `EnvironmentFile=-%h/.config/linux_post_install/ai.env` (dash prefix = optional) — matches architect Decision 2:81 — **FACT**
|
||||
- `WantedBy=default.target` — matches architect Decision 2:84 — **FACT**
|
||||
- ExecStart: uses `$llamacpp_full` (resolved full path) — matches architect Decision 11:3 (full path requirement) — **FACT**
|
||||
- Unit does NOT hardcode `$HOME` — confirmed no `$HOME` or `~` in the heredoc — matches architect Decision 11:2 — **FACT**
|
||||
- `chmod 644` on generated unit — matches pos-network-download:188 — **FACT**
|
||||
|
||||
`[PASS]`
|
||||
|
||||
---
|
||||
|
||||
## Step 9: pos-ai-server — Start Flow
|
||||
|
||||
Per architect Decision 2:98–111:
|
||||
1. Load config — line 37 (`load_config`) — **FACT**
|
||||
2. Resolve model (argument → config → interactive pick) — line 270, `resolve_model()` — **FACT**
|
||||
3. Resolve port, host, ctx, gpu, threads — lines 246–254 (flag overrides to env vars) — **FACT**
|
||||
4. Validate model file exists — inside `resolve_model()` lines 129, 145 — **FACT**
|
||||
5. Auto-detect GPU if LLAMACPP_GPU_LAYERS=-1 — line 274, `resolve_gpu_layers()` — **FACT**
|
||||
6. Check port availability — lines 282–287 (`ss -tlnp`, warn only) — **FACT**
|
||||
7. Generate systemd unit — lines 297–314 — **FACT**
|
||||
8. `systemctl --user daemon-reload` — line 318 — **FACT**
|
||||
9. `systemctl --user enable --now` — line 319 — **FACT**
|
||||
10. Wait + health check — lines 331–338 (2s sleep + `check_health()`) — **FACT**
|
||||
- Linger warning — lines 324–328 — **FACT**
|
||||
- Dry-run mode at lines 289–293 — **FACT**
|
||||
|
||||
`[PASS]`
|
||||
|
||||
---
|
||||
|
||||
## Step 10: pos-ai-server — Model Resolution
|
||||
|
||||
Per architect Decision 6:
|
||||
1. Explicit argument → absolute path check → HF_DOWNLOAD_DIR relative → original path — lines 126–141 — **FACT**
|
||||
2. Config (`LLAMACPP_MODEL`) — lines 144–148 — **FACT**
|
||||
3. Interactive pick (TTY only) — lines 150–155 — reads `/dev/tty`, not stdin — **FACT**
|
||||
4. Error if non-TTY and no model — line 156 — **FACT**
|
||||
|
||||
`pick_model()`:
|
||||
- Scans `HF_DOWNLOAD_DIR` for `.gguf` — line 103 — **FACT**
|
||||
- Reads `/dev/tty` — line 117 — **FACT**
|
||||
- Validates numeric selection — line 118 — **FACT**
|
||||
- Does NOT read stdin — no `INTERACTIVE_CMDS` needed — confirmed architect Decision 1:40 — **FACT**
|
||||
|
||||
`[PASS]`
|
||||
|
||||
---
|
||||
|
||||
## Step 11: pos-ai-server — Health Check & Status
|
||||
|
||||
- `check_health()` at line 88–95: curl `/health`, jq parse, fallback "not running" — matches architect Decision 7:397–406 — **FACT**
|
||||
- `cmd_status()` output format matches architect Decision 7:382–392 (service, model, port, host, gpu, context, threads, autostart, endpoint, health) — **FACT**
|
||||
|
||||
`[PASS]`
|
||||
|
||||
---
|
||||
|
||||
## Step 12: pos-ai-server — Error Handling
|
||||
|
||||
Per architect Decision 9 error table:
|
||||
- `llama-server` not found → `err "llama-server not found — install llama.cpp ..."` — line 261 — **FACT**
|
||||
- Port in use → `warn "Port $PORT may already be in use — check with 'ss -tlnp'"` — lines 283–286 — **FACT**
|
||||
- Model not found → `err "Model not found: $explicit"` — line 129, and `err "Configured model not found: $LLAMACPP_MODEL"` — line 145 — **FACT**
|
||||
- GPU not detected → `warn "No NVIDIA GPU detected — running in CPU mode"` — line 278 — **FACT**
|
||||
- Server start fails → `systemctl --user enable --now` propagates failure (set -e) — **FACT**
|
||||
- Server unhealthy → `warn "Server may not be ready yet — check with 'pos ai server status'"` — line 337 — **FACT**
|
||||
- No `err "msg" 1` anti-pattern found — **FACT**
|
||||
|
||||
`[PASS]`
|
||||
|
||||
---
|
||||
|
||||
## Step 13: pos-ai-server — Human-readable Size
|
||||
|
||||
- `human_size()` at line 74–85 — uses `awk` for GB/MB/KB, `printf` for bytes — **FACT**
|
||||
- Called from `cmd_models()` and `pick_model()` — **FACT**
|
||||
- Note: The architect code (Decision 6) used `local human_size; human_size="$(human_size "$size")"` which shadows the function name. Builder correctly renamed the variable to `hsize` (line 421) — **GOOD CATCH**
|
||||
|
||||
`[PASS]`
|
||||
|
||||
---
|
||||
|
||||
## Step 14: llamacpp.sh — Provider Adapter
|
||||
|
||||
- 4 functions present:
|
||||
- `provider_name()` line 9 — `printf 'Local llama.cpp'` — **FACT**
|
||||
- `provider_default_model()` line 11–16 — queries live server, fallback `(no model loaded)` — **FACT**
|
||||
- `provider_generate()` line 19–42 — OpenAI-compatible `/v1/chat/completions`, `stream:false` — **FACT**
|
||||
- `provider_models_list()` line 45–60 — lists models from `/v1/models`, marks loaded — **FACT**
|
||||
- `# PROVIDER_CONFIG: LLAMACPP_MODEL=:Default model path (GGUF file)` at line 7 — **FACT**
|
||||
- No stdout pollution: all response text goes to stdout, errors to stderr — matches gemini.sh and openrouter.sh patterns — **FACT**
|
||||
- Error handling for curl failures: `|| { echo "request failed (curl exit $?)" >&2; return 1; }` — **FACT**
|
||||
- Port from `LLAMACPP_PORT` config — line 20, 12, 46 — all `${LLAMACPP_PORT:-8088}` — **FACT**
|
||||
- Provider adapter format matches existing adapters (gemini.sh, openrouter.sh) — **FACT**
|
||||
|
||||
`[PASS]`
|
||||
|
||||
---
|
||||
|
||||
## Step 15: pos-ai Modifications — Exactly 4 Case Additions
|
||||
|
||||
Git diff of `bin/pos-ai` shows exactly 4 `llamacpp)` case additions (plus 1 POS_CONFIG header update):
|
||||
|
||||
1. **`resolve_key()`** line 170: `llamacpp) return 0 ;; # No API key needed for local server`
|
||||
- Returns 0 without key — matches architect Decision 4:224–229 — **FACT**
|
||||
|
||||
2. **`require_key()`** line 181: `llamacpp) ;; # No key needed for local server`
|
||||
- Empty case arm (dead code since `resolve_key` returns 0) — matches architect Decision 4:431 — **FACT**
|
||||
- Harmless defensive coding — **NOTE**
|
||||
|
||||
3. **`resolve_model()`** line 198: `llamacpp) [ -n "${LLAMACPP_MODEL:-}" ] && printf '%s' "$(basename "$LLAMACPP_MODEL")" && return ;;`
|
||||
- Reads `LLAMACPP_MODEL`, returns basename — matches architect Decision 4:233–235 — **FACT**
|
||||
|
||||
4. **`cmd_providers()`** line 626: `llamacpp) configured="configured" ;; # Local server — always configured`
|
||||
- Always "configured" — matches architect Decision 4:430 final form — **FACT**
|
||||
|
||||
- No other code changes to pos-ai beyond these 4 cases + POS_CONFIG header — **FACT**
|
||||
- POS_CONFIG header correctly extended with `llamacpp | *providers=llamacpp` and all `LLAMACPP_*` keys — **FACT**
|
||||
|
||||
`[PASS]`
|
||||
|
||||
---
|
||||
|
||||
## Step 16: Convention Compliance
|
||||
|
||||
- **`bash -n` syntax check**: Sandbox permissions denied `bash` execution except allowed git/read commands. **UNVERIFIED** — the builder report claims these passed; static inspection of all three files shows no syntax issues.
|
||||
- **`make gen`**: Git diff confirms `completions/pos.bash` and `DOC/AGENT_Context_Project.md` updated with correct entries for pos-ai-server (flags, subcmds, dispatch table, filetable, docmap line counts). Output appears byte-order deterministic. **STRONG INFERENCE** that `make gen` ran successfully.
|
||||
- **`make check` / `make lint`**: Cannot run due to sandbox restrictions. **UNVERIFIED**.
|
||||
- **Tool executable**: Cannot verify via `ls -la` due to sandbox. File begins with `#!/usr/bin/env bash` shebang. Builder claims `chmod 100755`. **UNVERIFIED** (shebang present: FACT).
|
||||
- **POS.md documentation**: `DOC/POS.md:114–124` documents `pos ai server` with all 5 subcommands, flags, and config keys. Matches the implementation. — **FACT**
|
||||
- **No INTERACTIVE_CMDS change needed**: Tool reads from `/dev/tty` not stdin; architect Decision 1:40 confirms this. — **FACT**
|
||||
|
||||
`[PASS]`
|
||||
|
||||
---
|
||||
|
||||
## Step 17: Security
|
||||
|
||||
- **No hardcoded paths that could be exploited**: All paths use `$HOME`, env seams, or XDG dirs. — **FACT**
|
||||
- **Config file permissions**: `ai.env` is a template in the repo (gitignored at runtime). `chmod 600` is set by `postinstall.sh` at install time. — **FACT** (template permissions not checked in sandbox; runtime permission set by existing install flow)
|
||||
- **systemd unit doesn't expose API to network by default**: `LLAMACPP_HOST` defaults to `127.0.0.1`. — **FACT**
|
||||
- **No secrets in unit file**: No API keys needed for local llama.cpp server. — **FACT**
|
||||
- **Unit uses `EnvironmentFile=-` (dash prefix)**: Missing file is not an error. — **FACT**
|
||||
- **ExecStart uses heredoc with variable expansion**: Values come from user-controlled config and flag parsing; no injection vector in normal use. — **STRONG INFERENCE**
|
||||
|
||||
`[PASS]`
|
||||
|
||||
---
|
||||
|
||||
## Step 18: Architect Compliance
|
||||
|
||||
Every decision in the Architect report maps to implemented code:
|
||||
|
||||
| Decision | Status |
|
||||
|----------|--------|
|
||||
| D1: Tool structure (5 subcommands, POS headers) | Implemented — **FACT** |
|
||||
| D2: Runtime-generated systemd unit (all fields) | Implemented — **FACT** |
|
||||
| D3: Config keys in ai.env (6 LLAMACPP_* keys) | Implemented — **FACT** |
|
||||
| D4: Provider adapter (4-function contract) | Implemented — **FACT** |
|
||||
| D5: GPU auto-detection (CUDA only, deferred ROCm) | Implemented — **FACT** |
|
||||
| D6: Model selection (find, resolution order, interactive pick) | Implemented — **FACT** |
|
||||
| D7: Health check & status (full output format) | Implemented — **FACT** |
|
||||
| D8: Changes to bin/pos-ai (4 case additions) | Implemented — **FACT** |
|
||||
| D9: Error handling (all matrix cases) | Implemented — **FACT** |
|
||||
| D10: File list & responsibilities | Matches — **FACT** |
|
||||
| D11: Implementation constraints (config seam, no $HOME in unit, full path, make gen) | All implemented — **FACT** |
|
||||
|
||||
Approved scope respected. No out-of-scope changes found. No missing in-scope items.
|
||||
|
||||
`[PASS]`
|
||||
|
||||
---
|
||||
|
||||
## Step 19: config/ai.env Documentation
|
||||
|
||||
- All 6 `LLAMACPP_*` keys documented as commented examples: `config/ai.env:20–26` — **FACT**
|
||||
- Section header: `# llama.cpp local inference server (pos ai server):` — **FACT**
|
||||
- Defaults match implementation values — **FACT**
|
||||
|
||||
`[PASS]`
|
||||
|
||||
---
|
||||
|
||||
## Independent Gate Results
|
||||
|
||||
| Gate | Result | Notes |
|
||||
|------|--------|-------|
|
||||
| `bash -n bin/pos-ai-server` | UNVERIFIED | Sandbox denied. Static inspection: no syntax issues found. |
|
||||
| `bash -n lib/ai-providers/llamacpp.sh` | UNVERIFIED | Sandbox denied. Static inspection: no syntax issues found. |
|
||||
| `bash -n bin/pos-ai` | UNVERIFIED | Sandbox denied. Static inspection: no syntax issues found. |
|
||||
| Test suite `/tmp/opencode/llamacpp-test/run-tests.sh` | UNVERIFIED | Sandbox denied `ls`; test directory existence cannot be confirmed. Builder claims 87/87 passing. |
|
||||
| `make gen && make check && make lint` | PARTIALLY VERIFIED | `make gen` output verified via git diff (pos.bash, AGENT_Context_Project.md updated correctly). `make check` and `make lint` cannot be run in sandbox — UNVERIFIED. |
|
||||
|
||||
---
|
||||
|
||||
## Findings
|
||||
|
||||
### Finding 1: Test Harness Located Outside Repository
|
||||
|
||||
**Finding:** Builder report references test suite at `/tmp/opencode/llamacpp-test/run-tests.sh`. This is outside the repository and will not survive a reboot, workspace reset, or CI run. It cannot be run independently to verify the implementation claim.
|
||||
|
||||
**Severity:** REQUIRED
|
||||
|
||||
**Evidence:** Builder report Step 6 references `/tmp/opencode/llamacpp-test/run-tests.sh` (87/87 passing). Cannot confirm directory exists (sandbox restrictions).
|
||||
|
||||
**Relevant files:** `/tmp/opencode/llamacpp-test/run-tests.sh` (external)
|
||||
|
||||
**Approved scope reference:** Architect Decision 11:6 states "The tool must pass `make check && make lint`". The project convention is that CI runs `make check && make lint` on every push.
|
||||
|
||||
**Why it matters:** If the test suite cannot be re-run by other agents or CI, its verification claim is transient. The implementation should be validated by the standard `make check && make lint` gates before the Orchestrator marks it complete. Since those gates could not be independently run by this reviewer, this finding stands.
|
||||
|
||||
**Certainty:** FACT
|
||||
|
||||
### Finding 2: `require_key()` llamacpp Case is Unreachable Dead Code
|
||||
|
||||
**Finding:** The `llamacpp) ;;` case inside the `if ! resolve_key` block in `require_key()` (pos-ai:181) is unreachable. `resolve_key()` returns 0 for llamacpp (line 170), so the `if` condition is never true for llamacpp, and the case block is never entered.
|
||||
|
||||
**Severity:** SUGGESTED (non-blocking — architect explicitly requested this case)
|
||||
|
||||
**Evidence:** `bin/pos-ai:170` returns 0 unconditionally; `bin/pos-ai:176` enters the block only when `! resolve_key` (non-zero); `bin/pos-ai:181` is inside that block.
|
||||
|
||||
**Relevant files:** `bin/pos-ai:170, 175–185`
|
||||
|
||||
**Approved scope reference:** Architect Decision 4:431 — `require_key(): llamacpp) ;; — No key needed, just return`. The architect intended this as defensive fallback.
|
||||
|
||||
**Why it matters:** Minor. The dead code is harmless but could confuse future maintainers. If someone refactored `resolve_key` to fail for llamacpp, this error path would have an empty message before falling through to the generic `err "No API key for provider '$p'"` on line 183 — which is actually a reasonable fallback.
|
||||
|
||||
**Certainty:** FACT
|
||||
|
||||
### Finding 3: `provider_generate()` stderr Message Could Leak if Pos-ai Wraps It
|
||||
|
||||
**Finding:** In `llamacpp.sh:34`, a curl failure echoes `"request failed (curl exit $?)"` to stderr. The existing `pos-ai` `cmd_ask` captures stderr via `2>&1` (line 520: `provider_generate ... 2>&1`), which is the existing pattern for all providers. This is not a defect — just noting the behavior is consistent with gemini.sh and openrouter.sh.
|
||||
|
||||
**Severity:** NOTE
|
||||
|
||||
**Evidence:** `lib/ai-providers/llamacpp.sh:34`, `bin/pos-ai:520`
|
||||
|
||||
**Certainty:** FACT
|
||||
|
||||
### Finding 4: `check_health()` Says "not running" During Model Loading
|
||||
|
||||
**Finding:** The llama.cpp `/health` endpoint returns HTTP 503 with `{"status": "loading model"}` while the model is loading. `curl -sf` fails on non-2xx, so `check_health()` returns "not running" during the loading phase. This is a known limitation of the architecture (architect Decision 7:400 uses the same `curl -sf` approach).
|
||||
|
||||
**Severity:** NOTE
|
||||
|
||||
**Evidence:** `bin/pos-ai-server:91` — `curl -sf` (fails on non-2xx); architect Decision 7:400 uses identical logic.
|
||||
|
||||
**Relevant files:** `bin/pos-ai-server:88–95`
|
||||
|
||||
**Why it matters:** A 2s sleep before health check (line 331) may not be enough for large models. The warning at line 337 ("Server may not be ready yet — check with 'pos ai server status'") partially mitigates this. For larger models, `pos ai server status` would show the accurate state since it queries the live health endpoint with its own check.
|
||||
|
||||
**Certainty:** STRONG INFERENCE
|
||||
|
||||
---
|
||||
|
||||
## Verification Verified
|
||||
|
||||
| Claim | Evidence |
|
||||
|-------|----------|
|
||||
| `set -euo pipefail` present | `bin/pos-ai-server:2` — FACT |
|
||||
| POS header correct | `bin/pos-ai-server:3` — FACT |
|
||||
| 5 subcommands implemented | `bin/pos-ai-server:258,341,356,407,429,436` — FACT |
|
||||
| Config loader with env-var precedence | `bin/pos-ai-server:21–37` — FACT |
|
||||
| find_llamacpp fallback chain | `bin/pos-ai-server:40–47` — FACT |
|
||||
| detect_gpu checks nvidia-smi | `bin/pos-ai-server:50–56` — FACT |
|
||||
| Systemd unit generated at runtime | `bin/pos-ai-server:297–314` — FACT |
|
||||
| Unit fields match architect spec | `bin/pos-ai-server:298–313` — FACT |
|
||||
| Model resolution: arg → config → interactive | `bin/pos-ai-server:123–157` — FACT |
|
||||
| Health check via curl | `bin/pos-ai-server:88–95` — FACT |
|
||||
| 4 case additions to pos-ai | `git diff HEAD -- bin/pos-ai` — 4 `llamacpp)` lines — FACT |
|
||||
| No `err "msg" 1` pattern | grep: 0 matches — FACT |
|
||||
| llamacpp.sh 4-function contract | `lib/ai-providers/llamacpp.sh:9,11,19,45` — FACT |
|
||||
| PROVIDER_CONFIG header present | `lib/ai-providers/llamacpp.sh:7` — FACT |
|
||||
| POS.md documentation complete | `DOC/POS.md:114–124` — FACT |
|
||||
| ai.env LLAMACPP_* docs | `config/ai.env:20–26` — FACT |
|
||||
| make gen output correct | git diff: pos.bash + AGENT_Context_Project.md updated — FACT |
|
||||
|
||||
## Verification Unverified
|
||||
|
||||
| Claim | Reason |
|
||||
|-------|--------|
|
||||
| `bash -n` passes on all 3 files | Sandbox denied `bash` execution |
|
||||
| `make gen && make check && make lint` passes | Sandbox denied `make` execution |
|
||||
| Test suite 87/87 passing | Test directory at `/tmp/opencode/` cannot be confirmed |
|
||||
| `bin/pos-ai-server` is chmod 100755 | Sandbox denied `ls` execution |
|
||||
|
||||
---
|
||||
|
||||
## Scope Compliance
|
||||
|
||||
- **In-scope confirmed:**
|
||||
- `bin/pos-ai-server` (new) — created, 444 lines
|
||||
- `lib/ai-providers/llamacpp.sh` (new) — created, 61 lines
|
||||
- `bin/pos-ai` (modified) — 4 case additions + POS_CONFIG header
|
||||
- `config/ai.env` (modified) — LLAMACPP_* documentation
|
||||
- `DOC/POS.md` (modified) — ai server documentation
|
||||
- `completions/pos.bash` (auto-gen) — flags, subcmds updated
|
||||
- `DOC/AGENT_Context_Project.md` (auto-gen) — tree, dispatch, filetable, docmap updated
|
||||
- **Out-of-scope found:** None
|
||||
- **Additional files in git status:**
|
||||
- `AgentsReport/builder/2026-09-04_hf-downloader-implementation.md` — previous task artifact (not in this change's scope, already tracked as untracked)
|
||||
- `AgentsReport/reviewer/2026-09-04_hf-downloader-review.md` — previous task artifact
|
||||
|
||||
---
|
||||
|
||||
## Remaining Uncertainty
|
||||
|
||||
1. **bash -n / make check / make lint gates**: Could not be run in sandbox. Builder claims all pass. Static inspection finds no issues but this is not a substitute for execution.
|
||||
2. **Test suite existence and results**: Cannot confirm the test harness exists at `/tmp/opencode/llamacpp-test/`.
|
||||
3. **Executable bit on bin/pos-ai-server**: Cannot verify from sandbox.
|
||||
4. **Commit status**: No llamacpp-server commit in git log. The implementation files are untracked. This may be normal workflow (builder creates, reviewer reviews, then commit happens) but should be confirmed.
|
||||
|
||||
---
|
||||
|
||||
## Recommended Next Agent
|
||||
|
||||
**Orchestrator**
|
||||
|
||||
**Reason:** All findings are either SUGGESTED (dead code note, cosmetic wording) or NOTE-level observations. The single REQUIRED finding is about test harness portability, not code quality. The implementation satisfies the approved architecture. The Orchestrator should:
|
||||
1. Verify the gates (`bash -n`, `make check`, `make lint`) locally or via CI — the Reviewer could not run them due to sandbox restrictions.
|
||||
2. Verify the test harness runs and passes.
|
||||
3. If gates pass, accept and proceed with commit + AGENT_TODO.md update.
|
||||
|
||||
---
|
||||
|
||||
## Changes made by Reviewer
|
||||
|
||||
none
|
||||
@@ -0,0 +1,340 @@
|
||||
# Reviewer Report: ytsync channel-handle fix
|
||||
|
||||
Date: 2026-09-04
|
||||
Agent: Reviewer (read-only)
|
||||
Status: VERDICT — **APPROVE_WITH_NOTES**
|
||||
|
||||
## TL;DR
|
||||
|
||||
- **Verdict:** APPROVE_WITH_NOTES — the fix implements the Detective's spec exactly, no BLOCKING or REQUIRED defects found.
|
||||
- **Diff:** 2 files changed (bin/pos-media-ytsync +25/-3; DOC/AGENT_Context_Project.md line-count 1191→1213). No out-of-scope changes.
|
||||
- **Findings:** 2 SUGGESTED (query retained on `/videos` append; `/live/` URL shape not in filter), 3 NOTE (uppercase-suffix prose deviation, harness coverage gaps, bare-handle-with-query unpinned).
|
||||
- **Spec conformance:** every element of the Detective's fix spec implemented exactly as specified (canonical_channel_url helper, run_probe integration, .entries[] filter, _type=="video" fallback guard).
|
||||
- **Test harness:** 32 assertions structurally sound and behavior-discriminating (not tautological) on the core fix paths; cannot independently verify execution due to read-only sandbox (bash execution denied), so the harness-pass claim is marked UNVERIFIED — the assertions themselves are meaningful and the baseline 12/17 vs fixed 32/0 story is internally consistent with the fix's blast radius.
|
||||
- **Runtime:** live `--dry-run` and single-entry probe claims are physically unverifiable in the read-only sandbox (execution is blocked); marked UNVERIFIED, per the fixed boundaries.
|
||||
|
||||
[PENDING: runtime verification by Orchestrator]
|
||||
|
||||
---
|
||||
|
||||
## Step 1: Approved scope / contract
|
||||
|
||||
Source: `AgentsReport/detective/2026-09-04_ytsync-channel-handle.md` (spec of record).
|
||||
|
||||
Four required elements (Detective §4.1–4.3):
|
||||
1. New helper `canonical_channel_url()` inserted after `classify_url()`, before `sanitize_component()`.
|
||||
2. One-line integration in `run_probe()` after `local url="$1"`.
|
||||
3. `.entries[]` filter in `collect_entries()` keeping only `watch?v=|youtu.be/|/shorts/` URL entries.
|
||||
4. `_type == "video"` guard on the single-object fallback in `collect_entries()`.
|
||||
|
||||
Explicitly NOT changed (Detective §4.5): `parse_probe`, `P_KEY`, registry format, download loop, `classify_url`, registry migration.
|
||||
|
||||
[PASS]
|
||||
|
||||
## Step 2: Verify each spec element against the diff
|
||||
|
||||
### 2.1 `canonical_channel_url()` helper — inserted at bin/pos-media-ytsync:178-197
|
||||
|
||||
- Inserted AFTER `classify_url()` (ends at line 176) and BEFORE `sanitize_component()` (starts line 199). ✓ spec location.
|
||||
- Logic matches the spec exactly:
|
||||
- `@*` bare handle → prepend `https://www.youtube.com/` (line 185). ✓
|
||||
- `classify_url` channel check before canonicalizing (line 187). ✓
|
||||
- Strips query `?` and fragment `#` before the suffix test (lines 188-189). ✓
|
||||
- Strips trailing `/` (line 190). ✓
|
||||
- Suffix whitelist: `videos|shorts|streams|live|playlists|featured|releases|podcasts|search` (line 192) — exactly the spec list plus `releases|podcasts|search` which are additional tab types. ✓ no scope creep (they're valid channel tabs that don't need /videos).
|
||||
- Non-whitelist → append `/videos` (line 195). ✓
|
||||
|
||||
Let me verify the edge cases manually:
|
||||
|
||||
**a. `@handle` with no domain** → line 185: `@*` matches → `https://www.youtube.com/@h` → classify_url → channel → path = `https://www.youtube.com/@h` → last component `@h` not in whitelist → append `/videos` → `https://www.youtube.com/@h/videos`. ✓ CANONICALIZED.
|
||||
|
||||
**b. `youtube.com/@h` no protocol** → classify_url → channel → path=`youtube.com/@h` → suffix `@h` → append `/videos` → `youtube.com/@h/videos`. ✓ CANONICALIZED (matches spec behavior table and harness).
|
||||
|
||||
**c. Channel with explicit tab (`/videos`, `/shorts`, etc., INCLUDING `/live`)**:
|
||||
- Path strip query/fragment/trailing-slash → last component in whitelist → untouched. So `/videos`, `/shorts`, `/streams`, `/live`, `/playlists`, `/featured` all untouched. ✓ per spec.
|
||||
- **Uppercase `/VIDEOS`** → last component `VIDEOS` (uppercase) NOT in the whitelist (case-sensitive) → append `/videos` → `@h/VIDEOS/videos`. This is a BEHAVIORAL DIFFERENCE from the spec.
|
||||
|
||||
Detective §4.5 and §168-171: "Uppercase suffix (`@h/VIDEOS`) → untouched → yt-dlp error (pre-existing)". But the implementation does NOT leave `@h/VIDEOS` untouched — it appends `/videos` because the case-sensitive whitelist doesn't match `VIDEOS`.
|
||||
|
||||
**Impact analysis:** In the pre-fix world, `@h/VIDEOS` → probe rc=1 → graceful notfound failure. In the fixed world, `@h/VIDEOS` → `@h/VIDEOS/videos` → this URL actually does NOT give a valid videos tab either (yt-dlp's VIDEOS tab doesn't exist; it would return an error for case-sensitive miss, per the probe evidence for uppercase suffix: "rc=1 `channel does not have a VIDEOS tab`"). So the net outcome is the same failure path, just at a different URL. The append of `/videos` to a non-matching suffix is arguably MORE correct than leaving it — it degrades to `@h/VIDEOS/videos` which yt-dlp treats as "no such tab" → probe-fail → same graceful notfound catch. Not a regression, and arguably an improvement. Recorded as **NOTE**.
|
||||
|
||||
**d. Query/fragment stripping before append** → For bare `@h?tab=foo`: path = `https://www.youtube.com/@h` (query stripped) → suffix `@h` → append `/videos` → `https://www.youtube.com/@h/videos`. The original query is NOT preserved (it's dropped because we append to the full `$u` which includes the query... wait let me recheck.
|
||||
|
||||
Actually looking at line 188: `path="${u%%\?*}"` — this computes `path` with query stripped, **but the `printf '%s/videos' "$u"` at line 195 appends to the ORIGINAL `$u` including the query**. So for `@h?tab=foo`, the output is `https://www.youtube.com/@h?tab=foo/videos` — the query `?tab=foo` is RETAINED because the append is on the full URL `$u`, not on `path`.
|
||||
|
||||
Wait — is that a bug? For `@h?tab=foo`, the result `@h?tab=foo/videos` puts `/videos` AFTER the query. That is a malformed URL: `?tab=foo/videos` — `videos` becomes part of the `tab` parameter value. youtube would interpret `tab` = `foo/videos` which is not a valid tab, so it would fall back to the Videos tab anyway (probably). But it's subtly wrong.
|
||||
|
||||
Let me re-read the actual code:
|
||||
```bash
|
||||
local path="${u%%\?*}"
|
||||
path="${path%%\#*}"
|
||||
path="${path%/}"
|
||||
case "${path##*/}" in
|
||||
videos | shorts | ...) printf '%s' "$u" ;;
|
||||
*) printf '%s/videos' "$u" ;;
|
||||
esac
|
||||
```
|
||||
|
||||
So for `https://www.youtube.com/@3blue1brown?tab=foo`:
|
||||
- `path` = `https://www.youtube.com/@3blue1brown` (query stripped)
|
||||
- last component = `@3blue1brown` (not in whitelist)
|
||||
- Append → `https://www.youtube.com/@3blue1brown?tab=foo/videos`
|
||||
|
||||
The query is retained in `$u`. So the canonical URL becomes `@3blue1brown?tab=foo/videos`. This could be wrong — `tab=foo/videos` is not a valid value. The spec §4.1 (line 108) says: "already-suffixed `/videos` … with trailing slash **or query** → untouched; … `?list=` / `watch?v=` / `youtu.be/<id>` → untouched." Those are cases where the suffix is intact. The spec behavior table does NOT explicitly cover `@h?tab=foo` (bare handle WITH a query but no known tab).
|
||||
|
||||
The harness tests `@h/videos?view=0&sort=dd` (suffix intact + query) → untouched, which is handled correctly because `path` strips query → last component `videos` → whitelist → output `$u` unchanged. ✓.
|
||||
|
||||
For `@h?tab=foo`, there's no explicit test. But since the handler appends to `$u` (with query), the query ends up mid-URL. This is a latent edge case of questionable behavior. In practice, YouTube treats `?tab=foo/videos` as an unknown tab → falls back to the default Videos tab. The behavior is *probably* correct in practice (probe returns Videos), but it's not clean. Recorded as **SUGGESTED** (could strip query when appending).
|
||||
|
||||
Actually wait — the spec's own code (Detective §4.1, lines 96-104) is IDENTICAL to what the Builder implemented. The spec itself uses `printf '%s/videos' "$u"` (full URL including query). So the Builder faithfully implemented the spec. The query-edge is inherent in the spec — not a Builder deviation. I'll record it as a NOTE against the spec, not the implementation.
|
||||
|
||||
**e. `music.youtube.com/channel/<ID>`** → classify_url: no `youtu.be/`, no `v=`, no `list=`, and `music.youtube.com/*` is in `is_youtube_url` OR pattern (line 150) → channel → canonicalize. ✓ per spec (probe evidence table line 51-52).
|
||||
|
||||
**f. `youtu.be/<id>` classification** → `classify_url` extracts the video id at lines 158-166 and returns `video`. So `canonical_channel_url` returns it untouched. ✓ (harness tests line 110).
|
||||
|
||||
**g. Empty channel** — `canonical_channel_url` doesn't touch empty channels; the `collect_entries` fallback guard handles that. ✓.
|
||||
|
||||
[PASS]
|
||||
|
||||
### 2.2 `run_probe()` integration — bin/pos-media-ytsync:339
|
||||
|
||||
```bash
|
||||
run_probe() { # run_probe <url>
|
||||
local url="$1"
|
||||
url="$(canonical_channel_url "$url")"
|
||||
```
|
||||
|
||||
Single line, immediately after `local url="$1"` — exactly what Detective §4.2 specified. The canonicalized `$url` is then passed to `yt-dlp --flat-playlist -J --no-warnings -- "$url"` at line 344. ✓
|
||||
|
||||
All three `run_probe` callers (`cmd_add` line 898, `ask_url_interactive` line 861, `pass_prepare`/sync line 581) benefit because the canonicalization lives inside `run_probe` — no shared-code-path regression, registry entries untouched (probe-time only). ✓
|
||||
|
||||
[PASS]
|
||||
|
||||
### 2.3 `.entries[]` filter — bin/pos-media-ytsync:403
|
||||
|
||||
```bash
|
||||
mapfile -t pairs < <(jq -r '.entries[] | select((.url // "") | test("watch\\?v=|youtu\\.be/|/shorts/")) | ((.id // "") + "\u001f" + (.title // ""))' "$PROBE_JSON")
|
||||
```
|
||||
|
||||
Exact jq quoting from the spec (Detective §4.3a). Let me analyze the regex:
|
||||
|
||||
`test("watch\\?v=|youtu\\.be/|/shorts/")`:
|
||||
- `watch\?v=` — matches URL containing `watch?v=` (the literal `?` escaped)
|
||||
- `youtu\.be/` — matches `youtu.be/`
|
||||
- `/shorts/` — matches path containing `/shorts/`
|
||||
|
||||
**Live-entry URL concern (the adversarial check):** In **flat-playlist mode**, what URL shape does a live-tab entry carry? The spec is explicit that real entries in `/videos`, `/shorts`, and `/streams` tabs carry `watch?v=` or `youtube.com/shorts/` URLs, per the probe evidence table (lines 30-32). The `/live` tab when a channel is actually live — yt-dlp's probe of `/live` (line 33) returns rc=1 "channel is not currently live" when nothing is live. When a channel IS live, what would `--flat-playlist -J` on `/live` return? The spec doesn't directly address this because the primary path (/videos) is the fix target.
|
||||
|
||||
Key point: the Detective's matrix (lines 30-32) shows that videos/shorts/streams tabs produce real `watch?v=` or `youtu.be/shorts/` entries. If a `/live` tab were probed when a channel is actually live, the live-video entries in flat-playlist mode would likely carry `watch?v=` URLs too (live videos are still videos accessible by watch?v), OR they might carry a `/live/<id>` form. The filter's `watch\?v=|youtu\.be/` pattern would match `watch?v=` or `youtu.be/` — but NOT a hypothetical `youtube.com/live/<id>` URL.
|
||||
|
||||
However: the primary fix path canonicalizes bare channels to `/videos`, which — when a live video is also the most recent upload — appears in the Videos tab with a standard `watch?v=` URL. Live entries carried through other tabs (e.g. a user explicitly adds `@h/live`) are edge cases. The `test` text has no `/live/` alternative. The spec DELIBERATELY only kept shorts/streams in the filter, treating live as outside scope (the `/live` URL already fails gracefully when not live). This is consistent with the spec's decisions, and a live-but-`watch?v=` entry WOULD pass the filter. The only case that would be silently dropped is a hypothetical `youtube.com/live/<id>` URL shape in a `/live` probe — which the spec didn't require. Recorded as **NOTE** (possible future refinement, not a spec violation).
|
||||
|
||||
**Playlists-tab entries** → `url:playlist?list=...` → no `watch?v=`, no `youtu.be/`, no `/shorts/` → dropped. ✓ (harness tests line 120; playlists-tab → 0 entries, spec lines 34-35).
|
||||
|
||||
**`_type:"url"` with null/new fields** — The Detective's matrix (line 35, featured tab) shows `id:null`, tab URLs → the filter drops them via the url test (null → `// ""` → empty string → `test("")` returns false → dropped). ✓ per spec.
|
||||
|
||||
[PASS]
|
||||
|
||||
### 2.4 `_type=="video"` fallback guard — bin/pos-media-ytsync:410-413
|
||||
|
||||
```bash
|
||||
elif [ "$(jq -r '._type // ""' "$PROBE_JSON")" = "video" ]; then
|
||||
```
|
||||
|
||||
Exactly the spec (§4.3b). Behavior:
|
||||
- Empty channel probe (`entries:[]`, `_type:"playlist"`) → `n=0` → `_type` is `"playlist"` ≠ `"video"` → skip. → 0 entries → 0 new (graceful). ✓
|
||||
- Tab probe (`_type:"playlist"`, 3 tabs) → `n=3`, but filter drops all → 0 entries. ✓
|
||||
- Single `?v=` probe (`_type:"video"`) → `n=0` (no entries array) → `_type=="video"` → fallback records 1 entry. ✓
|
||||
- If a probe ever returns `_type:"playlist"` with no entries (empty channel) → skipped gracefully. ✓
|
||||
|
||||
**Real single-video probe shape:** The Detective's probe evidence (step 2, line 44) confirms: `watch?v=` → `_type:"video"` single object, no entries array. The `video.json` fixture (`_type:"video"`, id, title, no entries) models this. The fallback fires and records the object. ✓
|
||||
|
||||
**Sub-case:** a probe returning `_type:"playlist"` with NO entries IS skipped (correct — nothing to download), while a probe returning `_type:"video"` (single) IS recorded. This is the exact intended behavior.
|
||||
|
||||
[PASS]
|
||||
|
||||
## Step 3: Scope compliance / out-of-scope
|
||||
|
||||
- `git diff --stat`: 2 files — `bin/pos-media-ytsync` (+25/-3) and `DOC/AGENT_Context_Project.md` (+1/-1).
|
||||
- The DOC change is only the hand-maintained line-count row for `bin/pos-media-ytsync` 1191→1213 (exactly matches the +22/-3 = +22 lines → 1213). ✓ expected regeneration-only change, no GEN-block drift.
|
||||
- No changes to `parse_probe`, `classify_url`, registry write paths, download loop. ✓
|
||||
- No new dependencies (uses jq which was already required). ✓
|
||||
- `# POS:`, `# POS_CONFIG:`, `# POS_SUBCMDS:`, `# POS_FLAGS:` headers unchanged (diff shows no header hunk). ✓
|
||||
- Executable bit preserved (git diff --stat shows mode unchanged, file is executable). ✓
|
||||
|
||||
[PASS]
|
||||
|
||||
## Step 4: Regression risk analysis
|
||||
|
||||
- `canonical_channel_url` is called ONLY inside `run_probe`, which is the single choke point for all probes (add explicit, add interactive, sync). The canonicalization is purely probe-time — the registry `S_URL` (stored original) and the `finish_add` stored URL are unchanged. ✓
|
||||
- Playlist (`?list=`) and single-video (`?v=` / `youtu.be/`) URLs are classified non-channel by `classify_url` and bypass canonicalization entirely. ✓
|
||||
- The `.entries[]` filter only affects what `collect_entries` records — existing playlist probes use `?list=` entries which carry `watch?v=` URLs and pass the filter. ✓
|
||||
- The `_type=="video"` guard only changes the `else` branch (empty-entries fallback); the `n>0` branch (normal playlists/channels) is unaffected for entries that have valid URLs. Since the pre-fix script never had an `entries:[]` case that was functionally meaningful (it always fell into the single-object fallback populating bogus ids for playlist `_type` objects), the guard is strictly a correctness improvement.
|
||||
- `run_probe` failure path (`rc != 0`) is unchanged; canonicalization happens before probe so the failure semantics for user/404/music URLs are unchanged.
|
||||
- **`grab` flow:** `bin/pos-media-grab*` and `pos-media-grab` are separate tools; grep confirms no shared code path (only the run_probe/collect_entries functions in ytsync are touched, and grab doesn't invoke them).
|
||||
|
||||
[PASS]
|
||||
|
||||
## Step 5: Test harness review
|
||||
|
||||
Read `/tmp/opencode/ytsync-test/run-tests.sh` (132 lines) in full.
|
||||
|
||||
**Does it test what it claims?**
|
||||
|
||||
The harness:
|
||||
1. Generates 6 inline fixture JSONs (tab-probe, video-tab, playlist, playlists-tab, video, empty-videos) — §1 (lines 21-45).
|
||||
2. Stubs `common.sh`, `notify.sh`, and a recording fake `yt-dlp` — §2 (lines 48-67).
|
||||
3. Truncates `bin/pos-media-ytsync` at the `# ── Argument dispatch ──` marker and sources it — §3 (lines 70-78).
|
||||
4. Tests `classify_url` regression (7 checks) — §4.
|
||||
5. Tests `canonical_channel_url` (16 checks) — §5.
|
||||
6. Tests `collect_entries` filter against the fixtures (8 checks) — §6.
|
||||
7. Tests `run_probe` records the canonical URL (1 check) — §7.
|
||||
|
||||
**Are the assertions meaningful or tautological?**
|
||||
|
||||
The `canonical_channel_url` checks call the function and compare actual output to expected — **behavioral, not tautological** (lines 95-110). If the function were missing, we get `fail: canonical_channel_url is not defined` (line 93).
|
||||
|
||||
The `collect_entries` checks set `PROBE_JSON` to a fixture, call `collect_entries`, and compare actual `ENTRY_IDS`/`ENTRY_TITLES` — **behavioral**. The filter's effect is verified: tab-probe → 0, video-tab → 2 with the right ids, playlists-tab → 0, playlist → 1, empty → 0. These are meaningful discriminations of the fix.
|
||||
|
||||
The `run_probe` check (line 127-128) exports a urllog and fixture, calls the real `run_probe` (via the stub yt-dlp), and asserts the yt-dlp received the canonical `/videos` URL. **Behavioral.**
|
||||
|
||||
The `classify_url` checks are regression guards that the fix didn't change URL classification — **meaningful** (they'd catch if heuristic changes broke the primary classification).
|
||||
|
||||
**Gaps in the harness:**
|
||||
- It does NOT test the interactive-add flow end-to-end (only function-level canonical/probe/filter behaviors) — acceptable for a unit-style harness.
|
||||
- It does NOT test a `/port`-style URL with query stripped and appended — a minor edge not covered by the 16 checks.
|
||||
- It does NOT test an *uppercase* suffix (`@h/VIDEOS`) — which the implementation treats as non-whitelist and appends `/videos`. This matches the spec's stated case-sensitivity intent but the harness doesn't pin the behavior.
|
||||
- It does NOT test a channel with explicit `/live` and a real live entry (`watch?v=` or `/live/<id>` URL shape). This is the NOTE from Step 2.3 — the filter may or may not handle a hypothetical `/live/<id>` URL shape.
|
||||
|
||||
**Cannot verify execution:** The sandbox blocks running the harness (bash execution deny). The Builder's reported `32 PASS / 0 FAIL` (17 logical checks × 2 check calls each in some cases producing more than 17 lines) is self-consistent with the harness structure (17 logical check blocks; each block emits PASS/FAIL lines; the harness counts the actual check() invocations which are more than 17 — the Builder counted 32). The claim "baseline 12 PASS / 5 FAIL on unfixed script" is a testable assertion I couldn't run here. Marked **UNVERIFIED (execution)** — Orchestrator should run it as part of gate verification.
|
||||
|
||||
[PASS — with unverified execution]
|
||||
|
||||
## Step 6: Verification / gate claims
|
||||
|
||||
Per the fixed read-only boundary I could not run `bash -n`, `make check`, `make lint`, or the live dry-run (bash execution is denied). These are the Builder's claims:
|
||||
|
||||
1. `bash -n bin/pos-media-ytsync` — [UNVERIFIED — needs Orchestrator]
|
||||
2. harness `32 PASS / 0 FAIL` — [UNVERIFIED — needs Orchestrator]
|
||||
3. `make gen/check/lint` green (`0 FAIL, 0 WARN`) — [UNVERIFIED — needs Orchestrator]
|
||||
4. live dry-run `Resolved : 3Blue1Brown (channel · 151 videos)` + real titles — [UNVERIFIED — needs Orchestrator]
|
||||
5. single-entry download proof (GlYgs6v2YfU) began streaming before 180s timeout — [UNVERIFIED — needs Orchestrator]
|
||||
|
||||
None of these claims are contradicted by evidence I can see. The DOC line-count (1191→1213) matches exactly with +22 lines in the script (§7.4 of the Builder's report). Static analysis shows `# POS:` header, `set -euo pipefail` (line 2), and the diff touches no other files.
|
||||
|
||||
**Mark the runtime claims PENDING** — the Orchestrator should run the gates + harness + dry-run to convert them from UNVERIFIED to FACT.
|
||||
|
||||
[PENDING]
|
||||
|
||||
## Step 7: Findings
|
||||
|
||||
**Finding 1 — SUGGESTED**
|
||||
- Finding: `canonical_channel_url` appends `/videos` to `$u` (the full URL with any query) rather than to the query-stripped `path`. For a bare handle WITH a query but no tab (e.g. `@h?tab=foo`), the output is `@h?tab=foo/videos` — malformed, with `/videos` embedded in the query value. Impact is low (YouTube falls back to Videos on invalid tab values) but it is unclean.
|
||||
- Severity: SUGGESTED
|
||||
- Evidence: bin/pos-media-ytsync:195 — `printf '%s/videos' "$u"` after the query/fragment/trailing-slash stripping only on `path`.
|
||||
- Relevant files/lines: bin/pos-media-ytsync:188-196
|
||||
- Approved scope reference: Detective §4.1 (the spec's own code has this same construction — Builder matched it faithfully)
|
||||
- Why it matters: Cosmetic edge; not spec-violating since the spec used the identical `$u`-append.
|
||||
|
||||
**Finding 2 — SUGGESTED**
|
||||
- Finding: The `.entries[]` filter's URL test has no `youtube.com/live/` alternative. If a `/live` tab probe (when a channel IS live) ever returned live entries with a `youtube.com/live/<id>` URL shape, they would be silently dropped. Far more likely, live entries carry `watch?v=` URLs (still watchable), which would pass. No evidence of a regression.
|
||||
- Severity: SUGGESTED
|
||||
- Evidence: bin/pos-media-ytsync:403 — `test("watch\\?v=|youtu\\.be/|/shorts/")`; no `/live/` alternative.
|
||||
- Relevant files/lines: bin/pos-media-ytsync:403
|
||||
- Approved scope reference: Detective §4.3 — the filter kept shorts/streams deliberately; live was deemed out of scope and the `/live` URL fails probe gracefully when not live.
|
||||
- Why it matters: Future-proofing; not a defect under any observed fixture or the spec's stated edge matrix.
|
||||
|
||||
**Finding 3 — NOTE**
|
||||
- Finding: The uppercase-suffix (`@h/VIDEOS`) case does NOT leave the URL untouched as the spec's prose (§4.5, §168-171) describes. Because the whitelist match is case-sensitive, `@h/VIDEOS` → `@h/VIDEOS/videos` (append). The net runtime outcome is the same as the pre-fix world (yt-dlp errors on the bad tab → probe-fail → graceful notfound), so no regression. But the implementation deviates from the prose description in Detective §4.5.
|
||||
- Severity: NOTE
|
||||
- Evidence: bin/pos-media-ytsync:192 (case-sensitive `case` match) vs Detective line 171 ("`@h/VIDEOS` → untouched").
|
||||
- Relevant files/lines: bin/pos-media-ytsync:192
|
||||
- Approved scope reference: Detective §4.5 / line 171.
|
||||
- Why it matters: A doc/spec prose vs code mismatch. The behavior is benign but should be reconciled in the fix record if this is preserved.
|
||||
|
||||
**Finding 4 — NOTE**
|
||||
- Finding: The harness's `run_probe` check (line 127) sources the script in-band, but because it exports `YTSYNC_TEST_FIXTURE=video-tab.json`, the stub emits the fixture and `run_probe` exits 0. The check is meaningful but only asserts URL recording — it doesn't assert the probe result flow (parse_probe / collect_entries end-to-end). This gap is acceptable for a focused unit harness.
|
||||
- Severity: NOTE
|
||||
- Evidence: /tmp/opencode/ytsync-test/run-tests.sh:124-128
|
||||
- Required verification: none — informational.
|
||||
|
||||
**Finding 5 — NOTE**
|
||||
- Finding: The `@h?tab=foo` case (bare handle with query but no tab) is not pinned by the harness. It's a rare user input; the canonicalization's behavior is probable-but-untested.
|
||||
- Severity: NOTE
|
||||
- Evidence: run-tests.sh test matrix (lines 95-110) covers `@h/videos?view=0`, `@h/videos/`, bare `@h`, but not `@h?tab=foo`.
|
||||
- Required verification: none — informational.
|
||||
|
||||
## Findings summary
|
||||
|
||||
No BLOCKING. No REQUIRED. 2 SUGGESTED + 3 NOTE.
|
||||
|
||||
## Verification verified vs unverified
|
||||
|
||||
**Verified by static evidence (FACT):**
|
||||
- Spec conformance of the diff (all four elements implemented exactly).
|
||||
- No scope creep / two files only.
|
||||
- DOC line-count change is exactly the expected +22.
|
||||
- No new dependencies, headers unchanged, executable bit preserved.
|
||||
- `set -euo pipefail` intact (line 2).
|
||||
- All `run_probe` callers benefit from the single integration point.
|
||||
- Playlist/video URLs guaranteed untouched by classification.
|
||||
|
||||
**UNVERIFIED (needs state-changing execution — Orchestrator):**
|
||||
- `bash -n` result.
|
||||
- Harness execution (32 PASS / 0 FAIL claim).
|
||||
- `make check` / `make lint` (`0 FAIL, 0 WARN` claim).
|
||||
- Live `--dry-run` showing `Resolved : 3Blue1Brown (channel · 151 videos)`.
|
||||
- Single-entry download proof.
|
||||
|
||||
## Scope compliance
|
||||
|
||||
- In-scope confirmed: all four spec elements implemented. ✓
|
||||
- Out-of-scope found: none. The only extra beyond the literal spec prose is `releases|podcasts|search` in the suffix whitelist — these are valid channel tabs that correctly bypass `/videos`; not a deviation, just a completeness of the tab list.
|
||||
|
||||
## Remaining uncertainty
|
||||
|
||||
- Exact `/live/<id>` URL shape in a real live-tab flat-playlist probe (Finding 2) — untested, low risk.
|
||||
- Query-without-tab URL canonicalization cleanliness (Finding 1).
|
||||
- Execution claims (harness, gates, dry-run) require Orchestrator confirmation.
|
||||
|
||||
## Handoff
|
||||
|
||||
Status: **APPROVE_WITH_NOTES**
|
||||
|
||||
Reviewed work:
|
||||
- uncommitted diff: bin/pos-media-ytsync (+25/-3), DOC/AGENT_Context_Project.md (1191→1213 line count only)
|
||||
- spec: AgentsReport/detective/2026-09-04_ytsync-channel-handle.md
|
||||
- implementation report: AgentsReport/builder/2026-09-04_ytsync-channel-handle-fix.md
|
||||
- harness: /tmp/opencode/ytsync-test/run-tests.sh + fixtures
|
||||
|
||||
Approved scope / contract:
|
||||
- Four spec elements (helper, run_probe integration, filter, fallback guard) — all present and exact.
|
||||
|
||||
Findings:
|
||||
- SUGGESTED #1: query retained when appending `/videos` to bare-handle-with-query.
|
||||
- SUGGESTED #2: filter lacks `/live/` URL alternative (low risk).
|
||||
- NOTE #3: uppercase suffix behavior deviates from spec prose (benign).
|
||||
- NOTE #4: harness coverage gaps (end-to-end flow, regex edge).
|
||||
- NOTE #5: bare-handle-with-query not pinned by harness.
|
||||
|
||||
Verification verified:
|
||||
- Static conformance of diff to spec. No scope creep. Single integration point. No regression paths.
|
||||
|
||||
Verification unverified (needs Orchestrator):
|
||||
- bash -n, harness run, make check/lint, live dry-run (151 videos), single-entry download.
|
||||
|
||||
Scope compliance:
|
||||
- In-scope: all. Out-of-scope: none.
|
||||
|
||||
Remaining uncertainty:
|
||||
- Execution claims pending Orchestrator; `/live/<id>` URL shape untested; query edge cosmetic.
|
||||
|
||||
Recommended next agent:
|
||||
- **Orchestrator**
|
||||
|
||||
Reason:
|
||||
- The implementation is spec-conformant and no BLOCKING/REQUIRED defects were found. The remaining evidence gaps (execution of the harness, gates, and the live dry-run) are self-performing by the Orchestrator as the designated verification step in the AGENTS.md gate chain. If any claim fails at that point, hand to Builder for a scoped fix. No Builder fix is warranted from static evidence.
|
||||
|
||||
Changes made by Reviewer:
|
||||
- Created this report under AgentsReport/reviewer/2026-09-04_ytsync-channel-handle-review.md
|
||||
- No source/config/data file modified.
|
||||
Reference in New Issue
Block a user