fix: review-driven hardening of pos ai hf/server + llamacpp provider
gates / consistency-and-conventions (push) Successful in 2m16s
gates / consistency-and-conventions (push) Successful in 2m16s
Adversarial review of the AI tools (commits 387f23f/0856b25) found 2 BLOCKING + 5 REQUIRED defects; all fixed: - pos-ai-hf --include/--exclude: bash-case glob filtering (array-safe, no jq regex interpolation, composes gguf->filename->include->exclude) - pos-ai-server: ExecStart rebuilt as single-line properly-quoted command (systemd_quote for executable + model path; systemd-analyze verify rc=0) - --branch/--revision aliased (last wins), dead BRANCH variable removed - parallel download drains all jobs: per-pid wait, honest 'X of Y files, N failed' summary, rc=1 on partial failure, no .hf-meta for half-downloaded models, EXIT-trap temp cleanup - detect_llama_version guarded; validate_requested_flags errors on unsupported explicit flags with version-aware message - pos ai hf cache [status|clear]: real implementation, fail-closed confirm - new bin/pos-ai-llamacpp thin forwarder + llamacpp shorthand in bin/pos-ai (pos ai llamacpp <subcmd> = pos ai --provider llamacpp <subcmd>) - docs synced: bin/pos-ai usage(), DOC/POS.md AI_PROVIDER row, howto/ai.md (adapter list, --provider backends, shorthand, providers table); gen regenerated (tree/dispatch/completions) Verified: bash -n all bin/pos*; make gen idempotent; make check green; make lint 0 FAIL, 0 WARN. Reviewer acceptance: APPROVE_WITH_NOTES (0 REQUIRED). Audit deliverables + agent reports included for context.
This commit is contained in:
Your Name
@@ -0,0 +1,124 @@
|
||||
# Reviewer Report — Acceptance review of Builder R1/R2 fixes + whole pending diff
|
||||
|
||||
Date: 2026-09-06
|
||||
Reviewer: independent (read-only) reviewer — final acceptance gate
|
||||
Reviewed refs: working tree over HEAD `0856b25` + untracked `bin/pos-ai-llamacpp`
|
||||
Inputs read: Builder R1/R2 fix report (`AgentsReport/builder/2026-09-06_r1r2-fixes.md`), prior reviews (rereview + full review), the full `git diff HEAD`, `bin/pos-ai-server`, `bin/pos-ai-hf` (full reads), `bin/pos-ai-llamacpp`, `bin/pos`, `bin/pos-ai`, `completions/pos.bash`, `AGENT_TODO.md`, doc diffs (`DOC/POS.md`, `DOC/howto/ai.md`, `DOC/AGENT_Context_Project.md`), AGENTS.md / `DOC/DEV.md` conventions.
|
||||
|
||||
## TL;DR
|
||||
|
||||
- **Status:** APPROVE_WITH_NOTES
|
||||
- **Verdict:** Both REQUIRED findings from the prior re-review are **FIXED with supporting evidence**. R1 (systemd ExecStart quoting) and R2 (partial-failure honesty) both PASS by direct code read + diff. The whole pending diff (wire-up, docs, hygiene, completions) is internally consistent and committable. No BLOCKING or REQUIRED findings remain.
|
||||
- **Defect counts this pass:** 0 BLOCKING, 0 REQUIRED, 0 SUGGESTED, 3 NOTE (1 carryover-scope item recorded, 2 low-risk observations).
|
||||
- **Read-only constraint:** gates (`bash -n`, `make gen`/`check`/`lint`) and the live probes were executed by the Builder/Orchestrator, not independently reproduced here (sandbox denies execution). Accepted per the Orchestrator's instruction not to re-run make; the live evidence (real `systemd-analyze verify` rc 0 + 16-token word-split assertion; 2-file forced-failure probe) is consistent with the code as read.
|
||||
|
||||
---
|
||||
|
||||
## Step 1: R1 — systemd ExecStart quoting (`bin/pos-ai-server`)
|
||||
|
||||
- `systemd_quote()` (`bin/pos-ai-server:388-392`): `value="${value//\"/\\\"}"` then `printf '"%s"'` — wraps in systemd double quotes and escapes any embedded `"` as `\"`. This matches systemd.service(5) double-quote rules (double quotes preserve whitespace; `\"` escapes a quote).
|
||||
- Use (`bin/pos-ai-server:445`): `exec_cmd="$(systemd_quote "$llamacpp_full") -m $(systemd_quote "$model") --port $PORT --host $HOST"`. Quoting applies **only** to the executable and the model path — the two tokens that may legally contain spaces. Plain flag/number tokens are appended unquoted (`--n-gpu-layers`, `--ctx-size`, `--threads`, and conditionally `--gpu-threads`, `--tensor-split`, `--batch-size`, `--ubatch-size`, `--temperature`, `--top-k`, `--top-p`, `--repetition-penalty`, `--kv-cache`, `--slots`, and bare `--mmap`/`--mlock`/`--metrics`/`--health`). No over-quoting.
|
||||
- Dry-run parity: `log "(dry-run) ExecStart: $exec_cmd"` (`bin/pos-ai-server:494`) uses the exact same `$exec_cmd` string later written to the unit (`ExecStart=$exec_cmd`, `bin/pos-ai-server:508`). Byte-identical by construction.
|
||||
- Unit validity: `ExecStart="/usr/bin/llama-server" -m "/home/me/My Models/m.gguf" --port 8088 --host 127.0.0.1 ...` is a single valid systemd ExecStart line. systemd splits on unquoted whitespace and honors the double-quoted tokens as single args, so a model path with spaces survives. This is the precise failure class R1 targeted — now resolved.
|
||||
- Sanity check on OTHER user-provided values appended raw: `$PORT`, `$HOST`, `$gpu_layers`, `$GPU_THREADS`, `$TENSOR_SPLIT`, `$BATCH_SIZE`, `$UBATCH_SIZE`, `$TEMPERATURE`, `$TOP_K`, `$TOP_P`, `$REPETITION_PENALTY`, `$KV_CACHE_SIZE`, `$SLOTS`. Each is a numeric or address/split token where a space is not a legal value (host = IP/hostname; tensor-split = comma/semicolon GPU list; the rest numeric). The only tokens where spaces are legitimate (filesystem paths) are the two that ARE quoted. No realistic spaced-value breakage remains. (Defense-in-depth could quote all of them, but that is not required and would not change behavior for legal inputs — NOTE 2.)
|
||||
- `systemd-analyze verify`: Builder reports rc 0 on a spaced-path unit and an asserted word-split of 16 tokens. Not re-run here (execution denied); the unit text as read is plausible and valid.
|
||||
|
||||
[PASS]
|
||||
|
||||
## Step 2: R2 — partial-failure honesty (`bin/pos-ai-hf`)
|
||||
|
||||
- **`failed_files` scoping**: `local failed_files=()` declared at `bin/pos-ai-hf:632` inside `cmd_download` (function scope). Populated ONLY in the parallel branch (`:671`, `:685`). The sequential branch never touches it, so it stays empty there.
|
||||
- **`.hf-meta` gated on zero failures**: `if [ "${#failed_files[@]}" -eq 0 ]; then` (`:727`) writes meta; `else` (`:742-744`) `warn "Not writing .hf-meta — ${repo_id} is incomplete (N file(s) failed)"`. No complete-meta is written after partial failure. Confirmed.
|
||||
- **Summary honesty**: failure branch (`:757-761`) `success_count=$((file_count - ${#failed_files[@]}))` and prints `📥 Downloaded: %s (%d of %d files, %d failed: %s)`. The success-only summary `(%d files, %s)` (`:762-766`) is in the `else`, so it cannot appear when any file failed. No misleading "Downloaded:" success line on partial failure. Confirmed.
|
||||
- **Exit rc 1**: `if [ "${#failed_files[@]}" -gt 0 ]; then return 1; fi` (`:773-775`). Reachable only from the parallel path (sequential never populates the array). Confirmed.
|
||||
- **Sequential path unchanged**: the `else` sequential single-file branch (`:700-721`) is not among the diff's changed lines — `if ! hf_download_with_progress ...; then warn "Failed to download $fname"; continue; fi` retains its warn-and-continue, rc-0 semantics. The Builder's "byte-for-byte unchanged" claim is consistent with the diff. Confirmed.
|
||||
- Builder's live probe output (`[2/2] Failed: model-fail.bin`; `Not writing .hf-meta`; `1 of 2 files, 1 failed: model-fail.bin`; `rc=1`; dir lacks `.hf-meta`; `list` empty; single-file sequential success/failure unchanged) matches all of the above by code read.
|
||||
|
||||
[PASS]
|
||||
|
||||
## Step 3: Whole pending diff — wire-up
|
||||
|
||||
- `bin/pos-ai-llamacpp` (7 lines) is a byte-for-byte mirror of `bin/pos-ai-gemini` (`bin/pos-ai-gemini:1-7`) with only the provider name changed (`gemini`→`llamacpp`); `# POS_SUBCMDS: ask chat models sessions capture` matches the gemini forwarder and `lib/ai-providers/llamacpp.sh` capabilities.
|
||||
- `bin/pos-ai:701-704` `llamacpp)` case → `exec "$0" --provider llamacpp "${args[@]}"`, mirroring the gemini/openrouter forwarder cases. usage() provider lists updated (`bin/pos-ai:42`, `:59`).
|
||||
- `bin/pos:269` INTERACTIVE_CMDS adds `ai-llamacpp` (chat reads stdin → tee-pipe guard) — required and consistent with the reverse lint rule.
|
||||
- `completions/pos.bash`: `_pos_subcmds[ai-llamacpp]="ask chat models sessions capture"` and `[ai]` list includes `llamacpp`; `_pos_flags[ai-hf]`/`[ai-server]` expanded to match the headers. All consistent.
|
||||
|
||||
[PASS]
|
||||
|
||||
## Step 4: Whole pending diff — docs & completions
|
||||
|
||||
- `DOC/POS.md`: ai section references the three forwarders incl. llamacpp (`:55`); `--provider` row `(gemini\|openrouter\|llamacpp)` (`:69`); backward-compat sentence (`:82`); AI_PROVIDER config row (`:87`); hf download row documents `--branch`/`--revision` alias + `--include`/`--exclude` glob (`:105`); hf `cache [status|clear]` row added (`:108`); server flags row expanded + version-aware validation note (`:122`). All factual vs the code.
|
||||
- `DOC/howto/ai.md`: adapter list, `--provider` backend list, backward-compat shorthand, and "Available providers" table row (llamacpp, `LLAMACPP_MODEL`) — factual vs `lib/ai-providers/llamacpp.sh`.
|
||||
- `DOC/AGENT_Context_Project.md`: generated blocks (docmap line shifts, tree row, dispatch row, selfcontained, filetable) internally consistent with headers. Filetable line counts match `wc -l` exactly: `pos-ai-hf` 1004, `pos-ai-server` 659, `pos-ai` 706, `pos-ai-llamacpp` 7, `completions/pos.bash` 313, `bin/pos` 302.
|
||||
- `AGENT_TODO.md`: all Done entries dated (2026-09-06 / 2026-09-05). No un-dated entries introduced.
|
||||
|
||||
[PASS]
|
||||
|
||||
## Step 5: Hygiene / secrets / exec bits
|
||||
|
||||
- No `/tmp/opencode` references in any source file (grep clean).
|
||||
- No stray debugging/temporary code: grep for `DEBUG|print_r|console.log|TODO|FIXME|HACK|probe|stub` hits only comment words ("version probe", "probe the resolved binary") and a pre-existing unrelated alias tmpfile — no debug blocks or commented-out code in the changed files.
|
||||
- No secret literals: `HF_TOKEN` is read at runtime from env/config (`bin/pos-ai-hf:27,187`) — no embedded key/token. The `# POS_CONFIG: ... HF_TOKEN=secret:...` header is the declared masking classification, not a value. No `sk-`/`AIza`/`gh*_` style literals in the changed files.
|
||||
- Tracked exec bits confirmed 100755 via `git ls-files -s` for `bin/pos`, `bin/pos-ai-hf`, `bin/pos-ai-server`; `bin/pos-ai`, `bin/pos`, `completions` tracked normally. The untracked `bin/pos-ai-llamacpp` exec bit **cannot be confirmed via git** (untracked; `stat` denied by sandbox) — see NOTE 1.
|
||||
|
||||
[PASS]
|
||||
|
||||
---
|
||||
|
||||
## Findings
|
||||
|
||||
1. **NOTE** — untracked `bin/pos-ai-llamacpp` exec bit is UNVERIFIED via git (untracked file; `stat` denied by sandbox). It is a byte-for-byte mirror of `bin/pos-ai-gemini` (a tracked 100755 forwarder), and both the Maintainer's earlier `stat` claim and the Builder report assert 100755. LOW RISK; the dispatcher's `make gen`/check gates (already green per Orchestrator) would catch a missing exec bit. No action required if `git add` preserves exec-bit from the on-disk mode.
|
||||
2. **NOTE** — additional user-provided flag tokens (`--host`, `--tensor-split`, numeric flags) are appended unquoted. Each is a token type where a space is not legal, so no realistic breakage; this matches the brief's intended scope (quote only path-capable tokens). Optional hardening, not required.
|
||||
3. **NOTE** — the two SUGGESTED items carried from the prior re-review (`--list` ignores `--include/--exclude`; bash glob case-sensitivity) remain outstanding. They were NOT REQUIRED findings in either the re-review or this acceptance brief, and the R1/R2 scope was intentionally constrained to the two REQUIRED defects. Recorded for a future pass, not a blocker.
|
||||
|
||||
---
|
||||
|
||||
## Per-item status
|
||||
|
||||
| Item | Status | Evidence |
|
||||
|------|--------|----------|
|
||||
| R1 — systemd ExecStart quoting | **PASS** | `systemd_quote` (:388-392) escapes+quotes; binary+model only (:445); dry-run shares exact `$exec_cmd` (:494); unit writes same (:508); other tokens space-legal-check (NOTE 2) |
|
||||
| R2 — partial-failure honesty | **PASS** | `failed_files` function-scoped (:632); meta gated (:727-744); honest summary (:757-761); rc 1 (:773-775); sequential path untouched |
|
||||
| Wire-up (llamacpp) | **PASS** | Byte mirror of gemini forwarder; dispatch case; INTERACTIVE_CMDS; completions |
|
||||
| Docs & completions | **PASS** | POS.md / howto/ai.md / AGENT_Context factual; filetable line counts all match `wc -l` |
|
||||
| Hygiene / secrets | **PASS** | No `/tmp/opencode`, no stray debug, no embedded secrets; tracked exec bits 100755 (NOTE 1 for untracked exec bit) |
|
||||
| Gates | **Verified via Orchestrator handoff** | bash -n OK, make gen idempotent, make check `check-sync: OK`, make lint `0 FAIL, 0 WARN` (not re-run here per instruction) |
|
||||
|
||||
---
|
||||
|
||||
## Verification verified
|
||||
|
||||
- R1 `systemd_quote` implementation, use-site, dry-run/unit byte-parity, and systemd-valid ExecStart form — FACT by code read + diff (all in-scope lines cited above).
|
||||
- R2 failed_files scoping, meta gating, honest summary, rc-1 exit, and unchanged sequential path — FACT by code read + diff.
|
||||
- Wire-up (forwarder mirror, dispatch case, INTERACTIVE_CMDS, completions) — FACT by read.
|
||||
- Docs factual vs code + adapters; filetable line counts match `wc -l` exactly — FACT.
|
||||
- No `/tmp/opencode`, no stray debug, no secret literals in changed files — FACT by grep.
|
||||
- Tracked exec bits 100755 — FACT via `git ls-files -s`.
|
||||
- FINAL_SUMMARY/IMPLEMENTATION_PLAN + audit files remain untracked (Orchestrator commit decision, out of review scope) — consistent with prior passes.
|
||||
|
||||
## Verification unverified
|
||||
|
||||
- Gate commands (`bash -n`, `make gen` x2, `make check`, `make lint`, `systemd-analyze verify`) and the live probes — NOT independently reproduced in this sandbox (execution denied). Accepted as Orchestrator-handoff evidence: claims are internally consistent, match the code as read, and the probe outputs match the expected post-fix behavior exactly.
|
||||
- Exec bit of untracked `bin/pos-ai-llamacpp` — UNVERIFIED via git (stat denied); asserted 100755 by Maintainer/Builder, healthy risk.
|
||||
|
||||
## Scope compliance
|
||||
|
||||
- In-scope, delivered: R1 (ExecStart quoting) and R2 (partial-failure honesty) — both confirmed.
|
||||
- In-scope, unchanged/documented: sequential single-file failure behavior preserved per the "don't touch" constraint.
|
||||
- Out-of-scope changes: none found in source. The two carried SUGGESTED items are recorded as NOTE 3 (outside R1/R2 scope).
|
||||
- No new flags/subcommands/config keys introduced by the R1/R2 pass (docs/completions refreshed only by `make gen`).
|
||||
|
||||
## Remaining uncertainty
|
||||
|
||||
- Gate/probe results rest on the Builder/Orchestrator's reported runs rather than an observed run in this review sandbox. The code-level evidence independently confirms each claim to the extent a static read allows; the only residue is empirical (a real `systemd-analyze verify` on this tree, a real forced-failure parallel download, a real spaced-path `server start`), which the Builder reports green.
|
||||
- Untracked `bin/pos-ai-llamacpp` exec bit.
|
||||
|
||||
## Recommended next agent
|
||||
|
||||
**Orchestrator**
|
||||
|
||||
**Reason:** The verdict is final — APPROVE_WITH_NOTES. R1 and R2 are fixed and verified at the code level; the pending diff is committable. The Orchestrator should commit the reviewed source set (decision on untracked plan/report files as previously), then close the workflow. The two carried SUGGESTED items (NOTE 3) can be scheduled as a future builder pass; neither blocks this commit.
|
||||
|
||||
## Changes made by Reviewer
|
||||
|
||||
none
|
||||
Reference in New Issue
Block a user