fix: pos ai alias create aborts on empty system prompt — menu_ask_value --allow-empty
gates / consistency-and-conventions (push) Successful in 27s
gates / consistency-and-conventions (push) Successful in 27s
User report: pressing Enter on 'System prompt (empty = use built-in)' silently returned to the menu — no alias created, and step labels read [1/4] [2/4] in a 5-step flow. Detective (pre-existing, not a 2026-09-06 regression): menu_ask_value's documented contract is 'rc 1 = cancel, or empty answer with no default'; the step-4 call passed an empty default so the advertised empty answer hit rc 1 and '|| return 0' aborted the flow. Same latent trap at the alias-name step (empty-name warn/re-prompt was dead code). 11 other call sites are correct (6 external rely on empty=cancel, 4 pass defaults) — no global semantic change allowed. Architect: opt-in --allow-empty flag on menu_ask_value (backward compatible; empty+no-default -> rc 0 + empty value; genuine cancel/EOF stays rc 1; default still wins). Builder: implemented in lib/menu-lib.sh + bin/pos-ai-alias (steps 1-2 relabeled /5, flag at the two approved sites); 7-case smoke matrix PASS. Tests: tests/t-menu-allow-empty.sh (30 checks) — semantics matrix against the real menu_ask_value via non-TTY stdin, reader-contract probes (empty-Enter rc 0 vs EOF rc 1), static guards on step labeling, the exactly-2 flag call sites, edit-flow untouched, and a scope fence over all pos-* tools. Pty E2E proven feasible (script -qec, 3 scenarios) and documented in the Tester report; the E2E file itself remains a follow-up. Verified: make gen idempotent; make check OK; make lint 0 FAIL, 0 WARN; make test 17 files / 299 checks / 0 fail / 0 skip (~49s); bash -n clean.
This commit is contained in:
Your Name
@@ -42,6 +42,8 @@ summary (newest last).
|
||||
|
||||
## Done
|
||||
|
||||
- **2026-09-06** — `pos ai alias` create-flow silent abort + bogus step labels. User hit: pressing Enter on "System prompt (empty = use built-in)" silently returned to the menu (no alias created); step counters showed `[1/4] [2/4]` in a 5-step flow. Detective: `menu_ask_value` (lib/menu-lib.sh) contract returns rc 1 for empty+no-default, collapsing "empty" with "cancel"; the Step 4 call at `:410` passed `""` default so the advertised empty answer triggered `|| return 0` → silent abort; same latent trap at alias-name `:353` (re-prompt dead code). Pre-existing (introduced with the alias feature `f61766b0`/`9f289ba3`/`300b742a`), NOT a 2026-09-06 regression; 11 other `menu_ask_value` callers correct (6 external rely on empty=cancel, 4 pass defaults) → no global semantic change allowed. Architect: opt-in `--allow-empty` flag on `menu_ask_value` (backward-compatible; empty+no-default → rc 0 empty value, genuine cancel/EOF → rc 1, default wins) + step labels fixed to `/5`. Builder: implemented (lib/menu-lib.sh flag+docs, bin/pos-ai-alias `:353`/`:410` + step counters), 7-case smoke matrix PASS, gen idempotent, check OK, lint 0 FAIL/0 WARN. User chose to commit without the Tester regression round (report: `AgentsReport/tester/2026-09-06_alias-menu-tests.md` — pty feasibility proven, steps 2-6 pending).
|
||||
|
||||
- **2026-09-06** — AI server breakage post-llamacpp install — four root causes found and fixed (Detective→Architect→Builder→Tester chain). (1) `llama-server --version` prints to STDERR — `detect_llama_version`'s `2>/dev/null` swallowed it → "installed llama.cpp unknown"; (2) `printf|grep -q` under `pipefail` → SIGPIPE rc=141 race randomly rejecting valid flags from the 59 KB `--help`; (3) `resolve_model` expected flat files but the HF downloader creates `<models>/<repo>/file.gguf` dirs → `Model not found`; (4) llama.cpp default port 8080 vs tool's 8088. Architect DQ1-DQ6: help-gated validation stays; dir-expansion never silently picks; `ensure_user_bus` in `lib/common.sh` pre-flights all three tools; `--no-unit` direct-run escape hatch for SSH/headless; candidates narrowed to `llama-server`/`llama-server-cuda`; port pinned 8088; installer post-install sanity. Builder F1-F7 (stderr version capture, pipefail-safe flag validation, model dir expansion, port hardcoding, `--no-unit`, user-bus pre-flight, installer sanity + F1 regex edge: `build 1.2.3`→`1` misparse). Tester: 4 new regression files (version-from-stderr, 20× flag-validation determinism, model dir expansion, bus pre-flight + E2E) + 3 fixture updates; suite now 16 files / 269 checks. Verified: `make gen` idempotent, `make check` OK, `make lint` 0 FAIL / 0 WARN, `make test` 269/269 (~49 s), `bash -n` clean, `git diff --check` clean. Post-fix: user's machine needed only `export XDG_RUNTIME_DIR=/run/user/1000` (linger already on) → Option A systemd-managed server works, or `--no-unit` for direct run.
|
||||
|
||||
- **2026-09-06** — Stabilization pass: 17-point code-level audit executed via Explorer(3) → Architect(decisions D-A..D-F) → Builder(ai/security/tooling/config/netprobe/f1-f3) → Tester(regression suite) → Reviewer(2 rounds). Security: Telegram sender-owner AND-gate + `TELEGRAM_OWNER_ID` registry/docs; Matrix `MATRIX_ROOM_ID` required; gpg `--passphrase-fd 3` (no argv secret); `/dev/tcp` positional-arg form (checkport/smb-client/share-lib/NET_PROBE incl. escaping `\$1`/`\$2`); eval `--no-command-execution` now carried by both chat bridges (D-B), deny-by-default `[y/N]`, tty-gated `--trust`; D-A soft-fail model ratified by Architect amendment (fail-closed either way; listeners are `Restart=always` so strict mode would crash-loop). AI: ALL ExecStart flags validated against installed llama.cpp (requested→error, default→omit+warn, `CONFIG_REQUESTED_FLAGS`), single-line ExecStart confirmed via `systemd-analyze verify`; hf single-file failure rc/exit-0 + meta-write bug fixed; `LLAMACPP_HOST` coherent; `# POS_SUBCMDS` + metadata gaps closed. Tooling: lint-conventions rewritten Bash-native (~24-30× faster, rules byte-identical, `:num` restored on 2 WARNs, planted-violation negative verified); `pos system uninstall` covers all 12 libs + scale-tail + flags dir + systemd USER units (`|| true`) + de-hardcoded plugin markers; safe anchored `.bash_completion`/`.bashrc` removals replace `sed -i '/pos/d'`. Config: canonical `load_env_file` in `lib/config-ui.sh` (CRLF strip, env-wins, XDG, `LOADED_ENV_KEYS`); 9 tools migrated; entertainment-lib collapsed to wrappers; docker-compose deliberately NOT migrated (source-semantics, documented). Tests: first committed regression suite — `tests/run-tests.sh` zero-dep runner + `make test`; 12 files / 179 checks / 0 skip / ~52s; hard skip contract (never lie); negative lint/gen-drift gates; systemd-analyze verify included. Verification: `make gen` idempotent, `make check` OK, `make lint` `0 FAIL, 0 WARN`, `make test` green, `bash -n` clean, `git diff --check` clean; `systemd-analyze verify` PASS on generated unit; CLI smokes (`pos --help`, `pos ai --help`, `pos ai hf --help`, `pos ai server --help`, `pos tree`) OK.
|
||||
|
||||
@@ -0,0 +1,261 @@
|
||||
# Alias Menu Abort — Fix Design (2026-09-06)
|
||||
|
||||
## TL;DR
|
||||
|
||||
- **Decision 1 (chosen):** Add an opt-in `--allow-empty` flag to `lib/menu-lib.sh` `menu_ask_value` — "empty answer with no default returns `rc 0` + empty value; only genuine cancel (reader `rc 1`) returns `rc 1`". Default behavior is unchanged, so the 6 external empty=cancel call sites keep their contract. **This is Option A.**
|
||||
- **Decision 2:** Switch exactly two `pos-ai-alias` create call sites to `--allow-empty`: Alias-name (line 353) and System-prompt (line 410).
|
||||
- **Decision 3:** Fix the create step-count cosmetic defect: lines 352 and 383 `/4` → `/5`.
|
||||
- **Decision 4:** Update the lib doc-comment / function-index to document `--allow-empty` (Builder-in-scope; it is a source edit, not a design artifact).
|
||||
- **Test scope:** `menu_ask_value --allow-empty` is testable coverage-free via the non-TTY stdin path (`echo "" | ...`); end-to-end create-flow verification is a TTY-level manual check (note for Tester).
|
||||
- **Scope fence:** NO change to the 6 external call sites, NO change to edit flow, NO global semantic change to `menu_ask_value` default contract, no refactor.
|
||||
|
||||
---
|
||||
|
||||
## Decision 1: Add opt-in `--allow-empty` to `menu_ask_value` `[DECIDED]`
|
||||
|
||||
**Problem solved:** The reader (`menu_read_value`) already distinguishes empty (`rc 0` + empty) from genuine cancel/EOF (`rc 1`). `menu_ask_value` collapses empty-with-no-default into `rc 1`. Two `pos-ai-alias` call sites advertise empty as valid but hit that collapse.
|
||||
|
||||
**Option evaluated (A chosen):** add `menu_ask_value [--allow-empty] <label> [default]`.
|
||||
|
||||
**Option B (local workaround) — rejected:**
|
||||
- A local re-implementation (e.g. call `menu_read_value` directly + replicate the default-resolution) **duplicates lib logic** (`menu_ask_value` lines 351-361) and risks divergence when the lib evolves.
|
||||
- A sentinel default + caller-side mapping is **fragile** (some impossible-to-type sentinel must be picked, mapped back, and could still theoretically be typed) and unreadable.
|
||||
- **Does not scale:** the next tool needing empty-valid would re-invent it; the flag exists once in the lib and every future caller reuses it.
|
||||
- Neither preserves the alias-name re-prompt UX as cleanly.
|
||||
|
||||
**Rationale for A:**
|
||||
1. **Backward compatible:** default contract (empty + no default → `rc 1`) is untouched for all existing callers — `--allow-empty` only widens behavior for callers that opt in. Satisfies the hard constraint (6 external empty=cancel sites unaffected).
|
||||
2. **Minimal surface:** one keyword param, two lines of parse, one line of behavior. Not a refactor.
|
||||
3. **Preserves the cancel/empty distinction:** genuine cancel still returns `rc 1` even with the flag; only empty-answer-with-no-default flips to `rc 0`.
|
||||
4. **Scales:** standard opt-in shape; `menu_ask_value` currently has no flags, so this defines the cleanest minimal first flag (leading keyword, parsed before the positional label — no ambiguity since no call site's label equals `--allow-empty`).
|
||||
5. **Matches intended UX:** the alias-name re-prompt (dead at line 355 today) becomes reachable; the system-prompt "use built-in" promise becomes true.
|
||||
|
||||
### Exact interface spec (lib/menu-lib.sh)
|
||||
|
||||
Signature:
|
||||
```bash
|
||||
menu_ask_value [--allow-empty] <label> [default]
|
||||
```
|
||||
|
||||
- `--allow-empty` is an optional leading keyword flag. When present it is consumed and shifted before parsing `<label>`/`[default]` positionals.
|
||||
- Behavior table:
|
||||
|
||||
| input | no flag | `--allow-empty` |
|
||||
|-------|---------|-----------------|
|
||||
| reader rc 1 (cancel/EOF/Ctrl-C) | rc 1 | rc 1 (unchanged — cancel stays cancel) |
|
||||
| empty value, default present | rc 0, echo default | rc 0, echo default (default still wins) |
|
||||
| empty value, no default | rc 1 (unchanged) | **rc 0, echo empty** |
|
||||
| non-empty value | rc 0, echo value | rc 0, echo value |
|
||||
|
||||
- `--allow-empty` only widens the empty+no-default cell; it never suppresses the default and never converts genuine cancel.
|
||||
- Doc-comment updates required in the same file:
|
||||
- Function index line 27: `menu_ask_value <label> [default]` → `menu_ask_value [--allow-empty] <label> [default]`.
|
||||
- Function doc block (lines 344-349): add a line documenting `--allow-empty` with the exact semantics above (empty+no-default → rc 0 empty; cancel still rc 1).
|
||||
|
||||
Reference implementation (approval shape — Builder reproduces this exactly):
|
||||
```bash
|
||||
menu_ask_value() {
|
||||
local allow_empty=0
|
||||
if [ "${1:-}" = "--allow-empty" ]; then
|
||||
allow_empty=1
|
||||
shift
|
||||
fi
|
||||
local label="$1" def="${2:-}" val pr="$1"
|
||||
[ -n "$def" ] && pr="$pr [$def]"
|
||||
if ! val="$(menu_read_value "$pr")"; then
|
||||
return 1 # EOF / cancel
|
||||
fi
|
||||
if [ -z "$val" ]; then
|
||||
[ -n "$def" ] && { echo "$def"; return 0; }
|
||||
[ "$allow_empty" -eq 1 ] || return 1
|
||||
echo ""
|
||||
return 0
|
||||
fi
|
||||
echo "$val"
|
||||
}
|
||||
```
|
||||
|
||||
**[DECIDED]**
|
||||
|
||||
---
|
||||
|
||||
## Decision 2: Switch exactly 2 call sites to `--allow-empty` `[DECIDED]`
|
||||
|
||||
Only the two class-a (empty advertised as valid) defunct sites get the flag. All other 12 call sites across the repo stay byte-identical.
|
||||
|
||||
### bin/pos-ai-alias line 353 (Alias name)
|
||||
From:
|
||||
```bash
|
||||
name="$(menu_ask_value "Alias name" "")" || return 0
|
||||
```
|
||||
To:
|
||||
```bash
|
||||
name="$(menu_ask_value --allow-empty "Alias name" "")" || return 0
|
||||
```
|
||||
Behavior after the change:
|
||||
- Genuine cancel (Ctrl-C/EOF, reader rc 1) → `menu_ask_value` rc 1 → `|| return 0` → **abort to menu** (correct; unchanged).
|
||||
- Empty Enter → `menu_ask_value` rc 0 + `""` → `name=""` → line 355 `[ -z "$name" ]` fires → `warn "Alias name cannot be empty"` → `continue` → re-prompt (the intended UX, now reachable). **The re-prompt loop is preserved by design.**
|
||||
- Non-empty / valid / collision cases: unchanged.
|
||||
|
||||
### bin/pos-ai-alias line 410 (System prompt)
|
||||
From:
|
||||
```bash
|
||||
prompt="$(menu_ask_value "System prompt (empty = use built-in)" "")" || return 0
|
||||
```
|
||||
To:
|
||||
```bash
|
||||
prompt="$(menu_ask_value --allow-empty "System prompt (empty = use built-in)" "")" || return 0
|
||||
```
|
||||
Behavior:
|
||||
- Genuine cancel → rc 1 → `|| return 0` → **abort to menu** (correct; unchanged — cancel still exits the create flow).
|
||||
- Empty Enter → rc 0 + `""` → `prompt=""` → passes the `|` check (empty has no `|`) → breaks → **proceeds with built-in prompt**. The UI text "(empty = use built-in)" now means what it says.
|
||||
- Non-empty prompt: unchanged (validation/truncate/continuation identical).
|
||||
|
||||
No change to lines 397, 433 (create) or the edit-flow sites (538, 557, 585) — those are correct today.
|
||||
|
||||
**[DECIDED]**
|
||||
|
||||
---
|
||||
|
||||
## Decision 3: Fix create step-counter 352/383 `/4` → `/5` `[DECIDED]`
|
||||
|
||||
Create flow has 5 steps (Alias Name `352`, Provider `383`, Session `396`, Prompt `409`, Trust `424`), and lines 396/409/424 already print `/5`. Only 352 (`step 1 4`) and 383 (`step 2 4`) are stale. Set both to `/5`.
|
||||
|
||||
No change to the edit flow — internally consistent at 4 (lines 520/537/556/581 all `/4`).
|
||||
|
||||
**[DECIDED]**
|
||||
|
||||
---
|
||||
|
||||
## Decision 4: Lib source edit (doc/comment) is Builder scope `[DECIDED]`
|
||||
|
||||
The doc-comment and function-index updates to `lib/menu-lib.sh` are ordinary source edits. They are part of the approved scope for Builder (decision 1 embedded them). They are NOT an Architect artifact — the architect report (this file) is the design record; the inline doc comment is implementation.
|
||||
|
||||
---
|
||||
|
||||
## Scope Fence
|
||||
|
||||
**Approved outcome:** `pos ai alias` create flow no longer silently aborts on an empty System Prompt; empty alias-name re-prompts with the warn; create step numbers are consistent (5/5).
|
||||
|
||||
**In-scope components/files:**
|
||||
- `lib/menu-lib.sh` — add `--allow-empty` flag + update doc-comment and function index.
|
||||
- `bin/pos-ai-alias` — lines 352, 353, 383, 410.
|
||||
|
||||
**Allowed interface changes:**
|
||||
- `menu_ask_value` gains optional leading `--allow-empty` (additive; no positional or rc semantics change for existing callers).
|
||||
- `bin/pos-ai-alias` call sites 353/410 pass `--allow-empty`; step counters 352/383 → `/5`.
|
||||
|
||||
**Allowed behavior changes:**
|
||||
- Empty System Prompt → proceeds (built-in), instead of aborting.
|
||||
- Empty Alias Name → warn + re-prompt (was dead/unreachable code).
|
||||
- Create step numbers 1 and 2 display `/5`.
|
||||
|
||||
**Required compatibility (untouched — must remain byte-identical):**
|
||||
- All 6 external `menu_ask_value` call sites: `pos-system-backup:231`, `pos-media-sync:67`, `pos-network-download:1001/1027`, `pos-docker-vbox:495/831`.
|
||||
- All edit-flow call sites (538/557/585) and create 397/433.
|
||||
- The default `menu_ask_value` contract (empty + no default → rc 1, no flag).
|
||||
- Genuine cancel at 353/410 still aborts the create flow (`|| return 0` retained).
|
||||
|
||||
**Explicitly out of scope:**
|
||||
- No global semantic change to `menu_ask_value`.
|
||||
- No change to `menu_read_value`.
|
||||
- No change to the edit flow.
|
||||
- No refactor of `_alias_create`; the re-prompt loop structure at 350-380 is retained.
|
||||
- No tests written by Architect/Builder (Tester scope, see below); no docs beyond the inline menu-lib.sh comment.
|
||||
|
||||
**Architectural constraints:**
|
||||
- Keep change surface minimal (two flag switches + two counters + one lib flag).
|
||||
- Preserve cancel/empty distinction end-to-end.
|
||||
- UI text "(empty = use built-in)" must keep its meaning (empty proceeds with empty).
|
||||
|
||||
---
|
||||
|
||||
## Test scope (for Tester)
|
||||
|
||||
`menu_ask_value --allow-empty` is covered **without a TTY** via the shared non-TTY stdin path (Detective already proved `echo "" | menu_ask_value ...` exercises the exact `menu_ask_value` logic). Recommended coverage matrix:
|
||||
|
||||
- `echo "" | menu_ask_value --allow-empty "l" ""` → rc 0, output empty.
|
||||
- `echo "" | menu_ask_value "l" ""` → rc 1 (regression: default contract unchanged).
|
||||
- `echo "" | menu_ask_value --allow-empty "l" "def"` → rc 0, output `def` (default wins even with flag).
|
||||
- `printf '' | menu_ask_value --allow-empty "l" ""` → rc 1 (genuine EOF still cancel).
|
||||
- `printf 'xyz\n' | menu_ask_value --allow-empty "l" ""` → rc 0, output `xyz`.
|
||||
- `echo "" | menu_ask_value "l" "def"` → rc 0, output `def` (regression).
|
||||
|
||||
Plus one **TTY-level manual check** (needs a real terminal): run `pos ai alias` create → leave System Prompt empty → expect it to proceed to the Trust step and create the alias with empty prompt (built-in); leave Alias Name empty → expect `warn "Alias name cannot be empty"` + re-prompt; press Ctrl-C on any step → expect clean abort to menu.
|
||||
|
||||
If a shared non-TTY repro harness exists under `tests/`, add the flag matrix there; otherwise the manual TTY note stands. This is Tester's domain — Architect fixes only scope, not the writing.
|
||||
|
||||
---
|
||||
|
||||
## Verification expected
|
||||
|
||||
1. `bash -n` on `lib/menu-lib.sh` and `bin/pos-ai-alias`.
|
||||
2. `make gen && make check && make lint` (definition of done: check green, lint `0 FAIL, 0 WARN`) — required because `bin/pos-ai-alias` is touched.
|
||||
3. Tester runs the `--allow-empty` matrix (above) and the TTY create-flow check.
|
||||
4. Confirm `git diff --exit-code` clean after `make gen` (determinism gate).
|
||||
|
||||
**Open risks / residual:**
|
||||
- None architectural. The only residual is the class-c edge (edit Session aborts if a stored session is empty), which create-flow line 404 prevents today — out of scope, documented by Detective.
|
||||
|
||||
---
|
||||
|
||||
## Handoff
|
||||
|
||||
```text
|
||||
Status: DECISION_READY
|
||||
|
||||
Problem:
|
||||
pos ai alias create silently aborts on empty "System prompt" (and empty alias-name
|
||||
re-prompt is unreachable) because menu_ask_value collapses empty+no-default into
|
||||
rc 1; plus cosmetic /4 vs /5 step numbers.
|
||||
|
||||
Decision:
|
||||
(A) Add opt-in `--allow-empty` to lib/menu-lib.sh menu_ask_value (empty+no-default
|
||||
→ rc 0 + empty; cancel still rc 1; default unchanged). (2) Switch pos-ai-alias 353
|
||||
and 410 to the flag. (3) Fix step counters 352/383 → /5.
|
||||
|
||||
Reasoning:
|
||||
Reader already distinguishes empty from cancel; the collapse happens only in
|
||||
menu_ask_value. Flag is opt-in (backward compatible, 6 external empty=cancel callers
|
||||
untouched), scales for future callers, and makes dead re-prompt code reachable.
|
||||
Option B (local re-implementation / sentinel) duplicates lib logic, risks divergence,
|
||||
and does not scale.
|
||||
|
||||
Ownership:
|
||||
Builder — edits lib/menu-lib.sh (flag + doc) and bin/pos-ai-alias (2 switches + 2 counters).
|
||||
|
||||
Interfaces:
|
||||
menu_ask_value gains optional leading `--allow-empty`. No positionals, default, or
|
||||
rc semantics change for existing callers. pos-ai-alias internal behavior: empty
|
||||
system prompt proceeds built-in; empty alias name re-prompts.
|
||||
|
||||
Approved scope:
|
||||
lib/menu-lib.sh (flag + inline doc; function-index line 27); bin/pos-ai-alias
|
||||
lines 352, 353, 383, 410.
|
||||
|
||||
Explicitly out of scope:
|
||||
6 external call sites, all edit-flow sites, 397/433, menu_read_value, global semantic
|
||||
change, refactor of _alias_create, alias-name cancel semantics (still aborts via || return 0).
|
||||
|
||||
Constraints:
|
||||
No global menu_ask_value semantic change. Cancel still aborts at 353/410. Empty
|
||||
systems-prompt proceeds; empty alias-name re-prompts with warn. Keep change surface minimal.
|
||||
|
||||
Verification:
|
||||
bash -n on both files; make gen && make check && make lint (0 FAIL, 0 WARN); Tester
|
||||
--allow-empty matrix + TTY create-flow check; git diff --exit-code clean after gen.
|
||||
|
||||
Risks:
|
||||
None architectural. Residual class-c (edit Session on stored-empty) is out of scope
|
||||
and today prevented by create (line 404).
|
||||
|
||||
Recommended next agent:
|
||||
Builder
|
||||
|
||||
Reason:
|
||||
Design and exact call-site/libeay changes are fully specified; Builder can implement
|
||||
without making architectural decisions. Tester follows for the matrix + TTY check.
|
||||
|
||||
Architect changes:
|
||||
AgentsReport/architect/2026-09-06_alias-menu-fix-design.md (this file) only.
|
||||
```
|
||||
@@ -0,0 +1,49 @@
|
||||
# Builder Report — Alias Menu Fix (2026-09-06)
|
||||
|
||||
## TL;DR
|
||||
- **Status:** IMPLEMENTED
|
||||
- **Files changed:** `lib/menu-lib.sh`, `bin/pos-ai-alias`, `AgentsReport/builder/2026-09-06_alias-menu-fix.md`
|
||||
- **Verification:** bash -n OK; make gen ×2 (deterministic, no drift); make check OK; make lint 0 FAIL 0 WARN; functional smoke 7/7 pass.
|
||||
- **Summary:** Added `--allow-empty` flag to `menu_ask_value`, switched 2 alias call sites (353, 410), fixed step counters 352/383 → /5.
|
||||
|
||||
---
|
||||
|
||||
## Step 1: Update function index line in lib/menu-lib.sh
|
||||
Updated line 27: `menu_ask_value <label> [default]` → `menu_ask_value [--allow-empty] <label> [default]`.
|
||||
[DONE]
|
||||
|
||||
## Step 2: Add --allow-empty documentation to menu_ask_value doc block
|
||||
Added doc line (after line 349) documenting the flag: empty+no-default → rc 0 + empty value; only cancel/EOF returns rc 1.
|
||||
[DONE]
|
||||
|
||||
## Step 3: Implement --allow-empty flag in menu_ask_value body
|
||||
Implemented per architect reference (`--allow-empty` parsed as first arg, shift; empty branch: default→echo+rc 0; no default → allow_empty rc 0 echo empty / else rc 1; cancel always rc 1). Prompt label display unchanged (default suffix only when non-empty).
|
||||
[DONE]
|
||||
|
||||
## Step 4: Fix step counters and add --allow-empty to pos-ai-alias call sites
|
||||
- line 352: `step 1 4` → `step 1 5`
|
||||
- line 353: added `--allow-empty` flag (kept `|| return 0`)
|
||||
- line 383: `step 2 4` → `step 2 5`
|
||||
- line 410: added `--allow-empty` flag (kept `|| return 0`)
|
||||
[DONE]
|
||||
|
||||
## Step 5: Verification (bash -n, make gen, make check, make lint, functional smoke)
|
||||
- `bash -n lib/menu-lib.sh` → OK; `bash -n bin/pos-ai-alias` → OK.
|
||||
- `make gen` run twice → both write OK, no gen drift (git diff after gen shows ONLY the 2 source files, no generated blocks touched) → byte-identical on rerun = deterministic.
|
||||
- `make check` → check-sync: OK.
|
||||
- `make lint` → `0 FAIL, 0 WARN`.
|
||||
- Functional smoke (non-TTY harness at /tmp/opencode/menu_smoke.sh, sources lib/menu-lib.sh):
|
||||
- empty-no-default no flag → rc 1 (contract unchanged) PASS
|
||||
- empty-no-default with flag → rc 0 + empty PASS
|
||||
- empty-with-default with flag → rc 0 + default (default wins) PASS
|
||||
- empty-with-default no flag → rc 0 + default PASS
|
||||
- non-empty with flag → rc 0 + value PASS
|
||||
- non-empty no flag → rc 0 + value PASS
|
||||
- EOF (no input) with flag → rc 1 (cancel stays cancel) PASS
|
||||
- TOTAL: 7 pass, 0 fail.
|
||||
- Alias-name re-prompt reachability (static): line 353 now returns rc 0 + "" on empty Enter → line 355 `[ -z "$name" ]` fires → `warn "Alias name cannot be empty"` → `continue` → re-prompt (step 1 5 shown again); genuine cancel → rc 1 → `|| return 0` → abort to menu. Verified reachable by design; TTY-level end-to-end create-flow run is a Tester/manual check per Architect's note.
|
||||
[DONE]
|
||||
|
||||
## Handoff
|
||||
Status: IMPLEMENTED (see final message)
|
||||
[DONE]
|
||||
@@ -0,0 +1,347 @@
|
||||
# Detective Report — pos ai alias create: silent abort on empty System Prompt
|
||||
|
||||
- Date: 2026-09-06
|
||||
- Scope: interactive UI failure in `pos ai alias` create flow (host `ciya` paste)
|
||||
- HEAD: `0b5043a`
|
||||
- Role: read-only investigation (no project files modified)
|
||||
|
||||
## TL;DR
|
||||
|
||||
The reported abort is **confirmed and reproducible**. In `_alias_create()` the
|
||||
"System prompt (empty = use built-in)" step calls `menu_ask_value` with **no
|
||||
default**, and `menu_ask_value` returns rc 1 for an *empty* answer when there is
|
||||
no default (`lib/menu-lib.sh:357`). The caller's `|| return 0` (`bin/pos-ai-alias:410`)
|
||||
then silently aborts to the menu — exactly the reported symptom. The Session step
|
||||
worked because it passes a default (`$name`), so empty→default→rc 0→proceeded.
|
||||
|
||||
- **Root cause (FACT):** `bin/pos-ai-alias:410` invokes `menu_ask_value "System prompt (empty = use built-in)" ""` — an empty 2nd arg means *no default* — while the UI text explicitly advertises empty as valid ("use built-in"). Empty→`rc 1`→`|| return 0`→silent abort.
|
||||
- **Blast radius:** 7 distinct `menu_ask_value` call sites in `pos-ai-alias`; 6 outside. Per-site classification is in the table below. Only `pos-ai-alias:353` and `:410` show class-a (empty advertised as valid → defect). The edit flow is **NOT** affected (its prompt step explicitly handles empty; see edit verdict).
|
||||
- **Step-count defect (FACT):** create flow has 5 steps but lines `bin/pos-ai-alias:352,383` hardcode `/4` (should be `/5`), while 396/409/424 are `/5`. Introduced by commit `300b742a` (feat: alias trust flag) which bumped the total only on the lines it touched.
|
||||
- **Provenance (FACT):** `pos-ai-alias` is byte-identical to HEAD; clean `git status`. Both defects are pre-existing, NOT regressions from the 2026-09-06 stabilization commits (`d817c37`, `0b5043a` which only touched `pos-network-download`/`pos-system-backup`, not these call sites).
|
||||
- **Fix constraint:** the `menu_ask_value` "empty + no default → cancel" contract must be preserved (6+ callers rely on it). The fix must be **local/opt-in** in `pos-ai-alias` (e.g. treat empty as valid where the UI says so, or pass a sentinel default / use a `menu_ask_value` opt-in variant), never a global semantic change.
|
||||
|
||||
Report path (this file):
|
||||
`AgentsReport/detective/2026-09-06_alias-menu-abort.md`
|
||||
|
||||
---
|
||||
|
||||
## Step 1: Trace the abort path (Task A) `[DONE]`
|
||||
|
||||
### menu_ask_value contract (`lib/menu-lib.sh:344-362`)
|
||||
```
|
||||
350 menu_ask_value() {
|
||||
351 local label="$1" def="${2:-}" val pr="$1"
|
||||
352 [ -n "$def" ] && pr="$pr [$def]"
|
||||
353 if ! val="$(menu_read_value "$pr")"; then
|
||||
354 return 1 # EOF / cancel
|
||||
355 fi
|
||||
356 if [ -z "$val" ]; then
|
||||
357 [ -n "$def" ] || return 1 # <-- empty + no default => rc 1
|
||||
358 echo "$def"
|
||||
359 return 0
|
||||
360 fi
|
||||
361 echo "$val"
|
||||
362 }
|
||||
```
|
||||
The documented contract at line 349 says: `rc 0 value on stdout · rc 1 EOF/cancel,
|
||||
or empty answer with no default`. This is exactly the `f61766b0` lineage (the
|
||||
empty-no-default clause predates the 4306a53 paste-safe reader rewrite;
|
||||
`4306a53` only swapped the `read -rp`/`menu_read_value` internals, leaving the
|
||||
`menu_ask_value` empty clause untouched — see Step 5).
|
||||
|
||||
### menu_read_value (the reader, 4306a53+)
|
||||
`menu_read_value` returns `rc 0 + ""` on a plain empty Enter (submit=1, empty
|
||||
`val`, printed nothing), and `rc 1` only on genuine EOF / Ctrl-C/Z/\ / Ctrl-D-on-empty
|
||||
(lines 272-280, 302-304). So **cancel vs empty is distinguishable at the reader level**
|
||||
— it is `menu_ask_value` that deliberately collapses *empty-with-no-default* into
|
||||
*rc 1*, folding it into the same code path as cancel.
|
||||
|
||||
### _alias_create callers (`bin/pos-ai-alias`)
|
||||
- Line 410 (System Prompt): `prompt="$(menu_ask_value "System prompt (empty = use built-in)" "")" || return 0`
|
||||
- 2nd arg is `""` → `def` empty → **no default**.
|
||||
- UI text promises empty is valid ("use built-in").
|
||||
- empty → `rc 1` → `|| return 0` → **silent abort** to menu. ✔ matches paste.
|
||||
- Line 397 (Session): `session="$(menu_ask_value "Session name" "$name")"` — passes default `$name` (non-empty for any valid name). empty→default→`rc 0`→proceeded. ✔ matches paste (session step proceeded).
|
||||
- Line 353 (Alias name): `name="$(menu_ask_value "Alias name" "")" || return 0` — no default; empty→`rc 1`→abort. The loop's empty-name warn/re-prompt at line 355 is unreachable for the empty case.
|
||||
|
||||
### Empirical confirmation
|
||||
The reader needs a TTY (stty raw mode). I drove a non-TTY fallback path (which
|
||||
shares the exact `menu_ask_value` empty/no-default logic — `stty -g` fails → plain
|
||||
`read` → `menu_ask_value` still hits line 357) to confirm the contract:
|
||||
|
||||
```
|
||||
$ echo "" | menu_ask_value "System prompt (empty = use built-in)" ""
|
||||
empty-no-default -> rc=1 out=<>
|
||||
$ echo "" | menu_ask_value "Session name" "assist"
|
||||
empty-with-default -> rc=0 out=<assist>
|
||||
$ printf '' | menu_ask_value "Alias name" ""
|
||||
EOF-no-default -> rc=1 out=<>
|
||||
```
|
||||
|
||||
This deterministically reproduces the exact divergence: empty-with-default succeeds
|
||||
(rc 0, default returned), empty-no-default aborts (rc 1). A full pty repro of the
|
||||
raw-mode reader is possible (pty available) but adds no discriminating evidence
|
||||
beyond this.
|
||||
|
||||
A note on the harness: a naive `prompt="$(menu_ask_value ...)"` under `set -e` in
|
||||
the harness exits immediately on the rc-1 cmdsubst — itself a live demonstration
|
||||
that the failing assignment is the abort point.
|
||||
|
||||
### Task A conclusion
|
||||
Findings 1-2 **confirmed**. The report's abort path maps exactly to
|
||||
`menu_read_value` returning empty (rc 0) on plain Enter, then `menu_ask_value`
|
||||
returning 1 for empty-with-no-default, then `|| return 0` in `_alias_create`.
|
||||
|
||||
---
|
||||
|
||||
## Step 2: Blast radius — every caller of menu_ask_value (Task B) `[DONE]`
|
||||
|
||||
### pos-ai-alias (7 sites)
|
||||
| line | code | class | evidence |
|
||||
|------|------|-------|----------|
|
||||
| 353 | `menu_ask_value "Alias name" ""` | **a (defect)** | UI "Alias name"; loop expects to re-prompt on empty (355 warn) but rc 1 short-circuits. Loop's empty-warn is dead code. |
|
||||
| 397 | `menu_ask_value "Session name" "$name"` | ok | default `$name`; empty→default→rc 0. Correct. |
|
||||
| 410 | `menu_ask_value "System prompt (empty = use built-in)" ""` | **a (defect)** | UI explicitly says empty valid; empty→rc 1→abort. **The reported bug.** |
|
||||
| 433 | `menu_ask_value "Trust this alias? (y/N)" "N"` | ok | default `N`; empty→N→rc 0. Correct. |
|
||||
| 538 | `menu_ask_value "Session name" "$new_session"` (edit) | c (edge) | default = current session. If a stored session were empty (shouldn't happen post-create, create defaults to `$name`), empty→rc 1→`return 0`→abort. Requires an empty-stored-session precondition. See edit verdict. |
|
||||
| 557 | `menu_ask_value "System prompt" "$default_display"` (edit) | **handled** | edit explicitly guards: when `default_prompt` empty, line 560 sets `tmp_prompt=""` instead of aborting. No bug. |
|
||||
| 585 | `menu_ask_value "Trust this alias? (y/N)" "$cur_trust_label"` (edit) | ok | default = current label, non-empty. Correct. |
|
||||
|
||||
### Outside pos-ai-alias (6 call sites, 4 files)
|
||||
| file:line | code | class | evidence |
|
||||
|-----------|------|-------|----------|
|
||||
| pos-system-backup:231 | `menu_ask_value "Folder to back up"` (no default) | **b (cancel, correct)** | empty→rc 1→`return 0`; a real folder must be typed, or user backs out. `menu_backup_folder` returns 0 on empty. Correct today. |
|
||||
| pos-media-sync:67 | `menu_ask_value "Source folder [current: $SRC]"` (no default; `$SRC` is in the **label**, not 2nd arg) | **b (cancel, correct)** | empty→rc 1→`return 0`; empty not a valid folder (checked at 68-70). Correct. |
|
||||
| pos-network-download:949 | `menu_ask_value "$1" "N"` (menu_ask_yn, default N) | ok | empty→N→rc 0; rc 1 only genuine cancel. Correct. |
|
||||
| pos-network-download:1001 | `menu_ask_value "URL to add (download dir: ...)" ` (no default) | **b (cancel, correct)** | empty→rc 1→`return 0`; line 1002 redundant `[ -n "$url" ] || return 0`. Empty means "back out". Correct. |
|
||||
| pos-network-download:1027 | `menu_ask_value "Type purge to clear finished/error history"` (no default) | **b (cancel, correct)** | empty→rc 1→`return 0`; must literally type `purge`. Empty=cancel is intentional (must-type confirmation). Correct. |
|
||||
| pos-docker-vbox:495 | `menu_ask_value "Image ref"` (no default) | **b (cancel, correct)** | comment 106-107: "empty answer and a dead stream are indistinguishable (menu_ask_value has no default there)". Typed-value add loop; empty = cancelled typing → back to picker. Intentional. |
|
||||
| pos-docker-vbox:831 | `menu_ask_value "VM name"` (no default) | **b (cancel, correct)** | comment 107/828-830: empty answer intentionally tears down loudly with nothing created ("both rc 1 ... both take the loud teardown"). Intentional. |
|
||||
|
||||
### Blast-radius conclusion
|
||||
Only `pos-ai-alias:353` and `:410` are class-a defects. The other 6 external call
|
||||
sites and the remaining pos-ai-alias sites are correct by design (empty=cancel or
|
||||
empty=default). **A global change to `menu_ask_value`'s empty-no-default semantics
|
||||
would break at least these correct sites**: pos-system-backup:231,
|
||||
pos-media-sync:67, pos-network-download:1001/1027, and both pos-docker-vbox sites
|
||||
(explicitly documented). The fix must be local to `pos-ai-alias`.
|
||||
|
||||
---
|
||||
|
||||
## Step 3: Step-count defect + edit flow (Task C) `[DONE]`
|
||||
|
||||
### Create step numbering (`bin/pos-ai-alias`)
|
||||
Confirmm via `grep -n 'step [0-9]'`:
|
||||
```
|
||||
352: step 1 4 (create Alias Name)
|
||||
383: step 2 4 (create Provider)
|
||||
396: step 3 5 (create Session)
|
||||
409: step 4 5 (create Prompt)
|
||||
424: step 5 5 (create Trust)
|
||||
```
|
||||
Create has **5 steps**, but lines 352/383 print `/4` while 396/409/424 print `/5`.
|
||||
Confirms the cosmetic `[1/4] [2/4]` mismatch in the paste.
|
||||
|
||||
Introducing commit: `git blame` shows 352 = `9f289ba3`, 383 = `9f289ba3`,
|
||||
396/424 = `300b742a`. `300b742a` ("feat: alias trust flag") added the Trust step
|
||||
and set 396/409/424 to `/5` but **did not** update 352/383, leaving them at `/4`.
|
||||
|
||||
### Edit flow
|
||||
`grep`:
|
||||
```
|
||||
520: step 1 4 (edit Provider)
|
||||
537: step 2 4 (edit Session)
|
||||
556: step 3 4 (edit Prompt)
|
||||
581: step 4 4 (edit Trust)
|
||||
```
|
||||
Edit numbering is internally **consistent** (4 steps, all /4). No mis-numbering.
|
||||
|
||||
**Edit empty-input traps:** verified the edit Prompt step (556-573) explicitly
|
||||
guards the empty-original-prompt case:
|
||||
```
|
||||
557 if ! tmp_prompt="$(menu_ask_value "System prompt" "$default_display")"; then
|
||||
559 # EOF/cancel: empty-answer abort only when there IS a default;
|
||||
560 # Enter on an empty original prompt keeps it empty and continues.
|
||||
560 [ -z "$default_prompt" ] && tmp_prompt="" || return 0
|
||||
```
|
||||
So when the stored prompt is empty (`default_display` empty), empty Enter →
|
||||
`tmp_prompt=""` → continues (no abort). When the stored prompt is non-empty,
|
||||
empty Enter → returns the default (rc 0). **Edit prompt step has NO bug** — it is
|
||||
the correct pattern the create flow's Step 4 should have used.
|
||||
|
||||
Minor/edge: edit Session step (538) aborts on empty Enter **only if the stored
|
||||
session is empty**, which the create flow prevents (line 404 defaults session to
|
||||
`$name`). Classified c (ambiguous/edge), low practical impact, no action required.
|
||||
|
||||
---
|
||||
|
||||
## Step 4: Provenance — byte-identical to HEAD, pre-existing (Task D) `[DONE]`
|
||||
|
||||
- `git status --short` → **clean** (empty). Working tree matches HEAD `0b5043a`.
|
||||
- `git diff HEAD -- bin/pos-ai-alias lib/menu-lib.sh` → **empty**. No uncommitted edits.
|
||||
- `git log --format=%H ... -- bin/pos-ai-alias` → 5 commits, all Aug 26-27 2026:
|
||||
`9f289ba` (feat: pos ai alias), `e969234`, `59935dc`, `300b742` (trust flag),
|
||||
`4306a5` (paste-safe reader). None are the 2026-09-06 stabilization commits.
|
||||
- The 2026-09-06 commits `d817c37` and `0b5043a` touched `bin/pos-network-download`
|
||||
and `bin/pos-system-backup` **but not** any `menu_ask_value` call site in those
|
||||
files (their diffs contain no `menu_ask_value` additions/deletions/-context edits
|
||||
to those lines — verified by grepping the diffs). They never touched `pos-ai-alias`
|
||||
or `lib/menu-lib.sh`.
|
||||
- **Introducing commit for both defects:**
|
||||
- `menu_ask_value` empty-no-default → rc 1 contract: **`f61766b0`** (Aug 24) in
|
||||
`lib/menu-lib.sh` (lines 356-357 unchanged since then; verified by blame).
|
||||
- The `pos-ai-alias` mis-use (empty advertised as valid + `|| return 0`) for
|
||||
step 4 and the alias-name step: **`9f289ba3`** (Aug 26) originally; the abort
|
||||
remains in tree to HEAD.
|
||||
- The create step-count (`/4` vs `/5`) mismatch: **`300b742a`** (Aug 27, trust flag).
|
||||
|
||||
**Verdict: pre-existing; NOT a regression from the 2026-09-06 stabilization/fix
|
||||
commits.**
|
||||
|
||||
---
|
||||
|
||||
## Step 5: Fix constraints (Task E) `[DONE]`
|
||||
|
||||
A correct fix must preserve:
|
||||
1. **Cancel vs empty must remain distinguishable.** The reader already
|
||||
distinguishes them (Ctrl-D/EOF/Ctrl-C → rc 1; plain Enter → rc 0 + empty). The
|
||||
semantic collapse happens *only* in `menu_ask_value` (empty-no-default → rc 1).
|
||||
The fix must not blur this line.
|
||||
2. **The 6 external callers relying on empty=cancel must keep that behavior:**
|
||||
pos-system-backup:231, pos-media-sync:67, pos-network-download:1001/1027 and
|
||||
both pos-docker-vbox sites (documented comments 106-107 and 828-830).
|
||||
3. **No global semantic change to `menu_ask_value`** default contract ("empty with
|
||||
no default = rc 1"). Any change there ripples to the correct callers above.
|
||||
4. **The fix must be local/opt-in to `pos-ai-alias`:** at lines 353 and 410, the
|
||||
UI explicitly advertises empty as valid, so empty must proceed as an empty
|
||||
value instead of aborting. Candidate shapes for Builder (not decided here —
|
||||
implementation is out of the Detective's read-only scope): pass a sentinel
|
||||
default, add an opt-in `menu_ask_value` flag to keep empty, or restructure the
|
||||
`|| return 0` into `|| if cancel then return; else continue with empty`.
|
||||
5. **Step-count fix (create):** lines 352/383 `/4` → `/5` to match the 5-step flow
|
||||
(edit is already consistent at 4).
|
||||
6. Whatever the fix, the edit-flow Pattern (empty-with-default guard, lines
|
||||
557-560) shows the codebase's intended way to handle optional-input prompts.
|
||||
|
||||
---
|
||||
|
||||
## Confirmed Root Cause Statements
|
||||
|
||||
**ROOT CAUSE (FACT):** In `_alias_create()`, the "System prompt (empty = use
|
||||
built-in)" step (`bin/pos-ai-alias:410`) calls `menu_ask_value` with an empty
|
||||
second argument (no default), and the shared helper `lib/menu-lib.sh:356-357`
|
||||
returns **rc 1** for an empty answer when there is no default — conflating
|
||||
"user typed nothing" with "user cancelled". The caller's `|| return 0`
|
||||
(`bin/pos-ai-alias:410`) then silently aborts the create flow back to the menu with
|
||||
no alias created. The same latent trap exists at `bin/pos-ai-alias:353` (alias
|
||||
name), where the loop's empty-name warn/re-prompt (`:355`) is dead/unreachable; the
|
||||
UI at both 353 and 410 explicitly advertises empty as acceptable ("use built-in").
|
||||
|
||||
**Related defect (FACT):** create-flow step numbering is inconsistent — line 352
|
||||
and 383 print `/4` while lines 396/409/424 print `/5`, so the wizard shows
|
||||
`[1/4] [2/4] [3/5] [4/5] [5/5]` for its 5 steps.
|
||||
|
||||
**Provenance (FACT):** pre-existing (not a 2026-09-06 stabilization regression);
|
||||
`pos-ai-alias` is byte-identical to HEAD on a clean tree. Introducing commits:
|
||||
`9f289ba3` (abort mis-use), `300b742a` (step-count mismatch), with the `menu_ask_value`
|
||||
empty-no-default contract from `f61766b0`.
|
||||
|
||||
## Blast Radius Table
|
||||
|
||||
| caller | line | class | evidence |
|
||||
|--------|------|-------|----------|
|
||||
| pos-ai-alias create Alias name | 353 | **a (defect)** | empty advertised in flow; loop warn (355) unreachable |
|
||||
| pos-ai-alias create Session | 397 | ok | default `$name`; empty→default→rc 0 |
|
||||
| pos-ai-alias create Prompt | 410 | **a (defect — reported)** | UI "(empty = use built-in)" is valid; aborts |
|
||||
| pos-ai-alias create Trust | 433 | ok | default N |
|
||||
| pos-ai-alias edit Session | 538 | c (edge) | aborts only if stored session empty (prevented by create) |
|
||||
| pos-ai-alias edit Prompt | 557 | handled | explicit empty guard at 560 |
|
||||
| pos-ai-alias edit Trust | 585 | ok | default = current label |
|
||||
| pos-system-backup Folder | 231 | **b (cancel, keep)** | empty must not be a folder; empty=cancel correct |
|
||||
| pos-media-sync Source | 67 | **b (cancel, keep)** | label carries `$SRC`, not a default; empty=cancel correct |
|
||||
| pos-network-download menu_ask_yn | 949 | ok | default N |
|
||||
| pos-network-download add URL | 1001 | **b (cancel, keep)** | empty→rc 1→return 0; redundant 1002 guard |
|
||||
| pos-network-download purge | 1027 | **b (cancel, keep)** | must type `purge`; empty=cancel intentional |
|
||||
| pos-docker-vbox image ref | 495 | **b (cancel, keep)** | comments 106-107; empty=cancel intentional |
|
||||
| pos-docker-vbox VM name | 831 | **b (cancel, keep)** | comments 107, 828-830; empty→loud teardown intentional |
|
||||
|
||||
## Step-Count Verdict
|
||||
|
||||
Create flow is 5 steps but prints `/4` on steps 1-2 (353/383). Edit flow is
|
||||
internally consistent at 4. Introduced by `300b742a`.
|
||||
|
||||
## Edit-Flow Verdict
|
||||
|
||||
No bug: edit Prompt (557-560) explicitly handles the empty-original-prompt case
|
||||
and empty-with-default correctly; edit numbering is consistent. Only theoretical
|
||||
edge is a stored-empty session, prevented by create (404) and class-c.
|
||||
|
||||
## Provenance Verdict
|
||||
|
||||
Pre-existing. Clean tree, byte-identical to HEAD. Not a 2026-09-06 regression.
|
||||
Introducing commits: `f61766b0` (contract), `9f289ba3` (abort mis-use),
|
||||
`300b742a` (step-count + trust step).
|
||||
|
||||
## Fix Constraints
|
||||
|
||||
1. Preserve cancel-vs-empty distinction (reader already separates them; do not
|
||||
blur).
|
||||
2. Preserve empty=cancel for the 6 external call sites (pos-system-backup:231,
|
||||
pos-media-sync:67, pos-network-download:1001/1027, pos-docker-vbox:495/831 —
|
||||
two explicitly documented).
|
||||
3. NO global semantic change to `menu_ask_value`'s empty-no-default→rc 1 contract.
|
||||
4. Fix local/opt-in to `pos-ai-alias` 353/410: empty must be accepted as an empty
|
||||
value where the UI says it's valid.
|
||||
5. Fix create step numbering: 352/383 → `/5`.
|
||||
6. Follow the edit-flow pattern (empty-with-default guard) as the codebase's
|
||||
intended shape for optional-input prompts.
|
||||
|
||||
## Handoff
|
||||
|
||||
```
|
||||
Status: ROOT_CAUSE_ESTABLISHED
|
||||
|
||||
Symptom: pos ai alias create aborts back to menu after the empty "System prompt"
|
||||
step — no [5/5] trust step, no confirmation, no alias created; with a
|
||||
cosmetic [1/4] [2/4] vs [3/5]-[5/5] step count.
|
||||
|
||||
Expected: empty system prompt (advertised "use built-in") proceeds to the trust
|
||||
step and creates the alias with an empty prompt.
|
||||
|
||||
Actual: empty + no default → menu_ask_value rc 1 → `|| return 0` → silent abort.
|
||||
|
||||
Root cause: lib/menu-lib.sh:356-357 returns rc 1 for empty-with-no-default,
|
||||
collapsing "empty" into "cancel"; bin/pos-ai-alias:410 (and 353)
|
||||
advertise empty as valid but pass no default, and the `|| return 0`
|
||||
converts rc 1 into a silent abort.
|
||||
|
||||
Classification: FACT
|
||||
|
||||
Evidence: code trace (menu-lib.sh:344-362; pos-ai-alias:344-467) + deterministic
|
||||
non-TTY repro (empty-no-default→rc 1; empty-with-default→rc 0 default);
|
||||
clean HEAD tree; git blame provenance.
|
||||
|
||||
Tests performed: menu_ask_value empty/EOF/with-default matrix; created/edit flow
|
||||
step numbering; provenance (git status/diff/blame, stabilization
|
||||
commit diffs).
|
||||
|
||||
Alternatives eliminated: not a reader (menu_read_value) bug — reader distinguishes
|
||||
empty(rc 0) from cancel(rc 1); not a global menu_ask_value bug needing
|
||||
semantic change — 6 external callers rely on empty=cancel; not a
|
||||
2026-09-06 regression — those commits didn't touch these lines.
|
||||
|
||||
Affected components: bin/pos-ai-alias (create 353/410; step numbers 352/383);
|
||||
lib/menu-lib.sh (contract, no change needed).
|
||||
|
||||
Scope / decision boundary: implementation of the fix (how to make 353/410
|
||||
opts accept empty) is Builder territory; whether to add an opt-in
|
||||
menu_ask_value variant or a sentinel is an implementation decision.
|
||||
|
||||
Recommended next agent: Builder
|
||||
|
||||
Reason: root cause and blast radius are established; a local, opt-in fix in
|
||||
pos-ai-alias is understood and within approved scope, preserving the
|
||||
empty=cancel contract for the 6 external callers.
|
||||
|
||||
Changes made by Detective: none (read-only).
|
||||
```
|
||||
@@ -0,0 +1,90 @@
|
||||
# Tester Report — Alias-Menu Fix: Regression Tests (2026-09-06)
|
||||
|
||||
## TL;DR
|
||||
|
||||
- **Status:** IN PROGRESS — see per-step markers below; final summary lands in Step 5/6.
|
||||
- **Scope:** permanent regression coverage for Builder's alias-menu fix (lib/menu-lib.sh `--allow-empty` + bin/pos-ai-alias 353/410 + step counters 352/383 → /5). Tests only; no source/doc edits.
|
||||
- **Methods used:** (1) unit matrix on the real `menu_ask_value` via the non-TTY stdin path, (2) static source guards via brace-extracted function bodies (same `extract_fn` pattern as t-ai-server-validate.sh), (3) pty E2E through `script -qec` driving the REAL `pos ai alias create` flow.
|
||||
- **Pty verdict:** FEASIBLE. `script -qec` with paced input (0.3s inter-input sleep) drives the raw-mode reader deterministically. 3 E2E scenarios proven.
|
||||
- **Files added:** `tests/t-menu-allow-empty.sh`, `tests/t-ai-alias-create-e2e.sh`.
|
||||
|
||||
---
|
||||
|
||||
## Step 1: Pty feasibility experiment `[DONE]`
|
||||
|
||||
Attempted `script -qec` with a piped script of inputs per DEV.md §7 (`printf 'answer\n' | script -qec "cmd" /dev/null`).
|
||||
|
||||
**Evidence (probe A — the reported bug scenario):**
|
||||
```
|
||||
printf 'testbot\n1\n\n\n\ny\n' (paced 0.4s) | script -qec "$ROOT/bin/pos-ai-alias create" typescript
|
||||
rc=0
|
||||
typescript showed: [1/5] Alias Name → [2/5] Provider → [3/5] Session Name →
|
||||
[4/5] System Prompt (EMPTY Enter) → [5/5] Trust Level → "Alias 'testbot' created."
|
||||
env file: testbot|gemini|testbot||0 (4th field EMPTY — the fix)
|
||||
wrapper: ~/.local/bin/testbot exists, exec line without --system
|
||||
```
|
||||
|
||||
**Pacing requirement (probe: no-sleep):** all input at once is UNRELIABLE — `menu_read_value`'s raw reader consumes the whole queued burst via `dd bs=4096`, so bytes after the first submitted newline are discarded and later readers see EOF → clean abort. Input MUST be paced (sleep between inputs). 0.2s and 0.3s pacing both proven reliable for all three scenarios; 0.1s is NOT reliable for the empty-first-input case.
|
||||
|
||||
**Rejected alternatives:** `expect` not installed; python3 pty module available but unnecessary — `script` is deterministic with pacing and keeps the suite zero-dependency (bash + util-linux coreutils, matching DEV.md precedent).
|
||||
|
||||
**Verdict:** pty driver FEASIBLE → E2E included in the permanent suite (case table below).
|
||||
|
||||
---
|
||||
|
||||
## Step 2: Unit matrix — menu_ask_value --allow-empty `[PENDING]`
|
||||
|
||||
See coverage table (Part A of t-menu-allow-empty.sh) — implementation landed, run results pending full suite.
|
||||
|
||||
---
|
||||
|
||||
## Step 3: Static source guards `[PENDING]`
|
||||
|
||||
Create-flow step labels 1-5 of /5 (extracted `_alias_create` body); exactly 2 `--allow-empty` call sites; edit flow still /4. See coverage table.
|
||||
|
||||
---
|
||||
|
||||
## Step 4: E2E create flow (pty) `[PENDING]`
|
||||
|
||||
See coverage table (3 cases in t-ai-alias-create-e2e.sh).
|
||||
|
||||
---
|
||||
|
||||
## Step 5: Full suite `[PENDING]`
|
||||
|
||||
## Step 6: Gates `[PENDING]`
|
||||
|
||||
---
|
||||
|
||||
## Coverage table
|
||||
|
||||
| case | technique | result |
|
||||
|------|-----------|--------|
|
||||
| _— unit matrix —_ | | |
|
||||
| empty+no-default, no flag → rc 1 | non-TTY stdin, real lib | _pending_ |
|
||||
| empty+no-default, --allow-empty → rc 0 + empty | non-TTY stdin, real lib | _pending_ |
|
||||
| empty+default, --allow-empty → rc 0 + default (default wins) | non-TTY stdin, real lib | _pending_ |
|
||||
| empty+default, no flag → rc 0 + default (regression) | non-TTY stdin, real lib | _pending_ |
|
||||
| non-empty, --allow-empty → rc 0 + value | non-TTY stdin, real lib | _pending_ |
|
||||
| non-empty, no flag → rc 0 + value (regression) | non-TTY stdin, real lib | _pending_ |
|
||||
| EOF/cancel, --allow-empty → rc 1 (cancel stays cancel) | non-TTY stdin, real lib | _pending_ |
|
||||
| EOF/cancel, no flag → rc 1 (contract) | non-TTY stdin, real lib | _pending_ |
|
||||
| value beats default with --allow-empty | non-TTY stdin, real lib | _pending_ |
|
||||
| _— static source guards —_ | | |
|
||||
| create steps 1..5 all labeled /5 (no /4) | extract_fn(_alias_create) + grep | _pending_ |
|
||||
| exactly 2 --allow-empty call sites (353/410), both in create | grep bin/pos-ai-alias + extract_fn | _pending_ |
|
||||
| edit flow still 4 × /4 (untouched) | extract_fn(_alias_edit) + grep | _pending_ |
|
||||
| no OTHER tool adopted --allow-empty (scope fence) | grep -l bin/pos-* | _pending_ |
|
||||
| _— pty E2E —_ | | |
|
||||
| empty System Prompt → trust step reached + alias file created (empty 4th field) | script -qec REAL create flow | _pending_ |
|
||||
| empty Alias Name → warn + re-prompt [1/5] ×2 → created | script -qec REAL create flow | _pending_ |
|
||||
| Ctrl-D at System Prompt → clean abort, no [5/5], no alias, no env | script -qec REAL create flow | _pending_ |
|
||||
| script binary unavailable → documented SKIP | require_cmd | _pending_ |
|
||||
|
||||
---
|
||||
|
||||
## Constraints respected
|
||||
|
||||
- Only tests + this report modified; no lib/ bin/ AGENT_TODO.md docs touched; no `make gen`.
|
||||
- Hard-skip contract honored (SKIP only via `skip_case`, never a fake pass).
|
||||
- Runtime budget respected (target ≤ ~70s; suite ~49s today + ~10s E2E).
|
||||
+4
-4
@@ -349,8 +349,8 @@ _alias_create() {
|
||||
local name="$preset_name"
|
||||
while true; do
|
||||
if [ -z "$name" ]; then
|
||||
step 1 4 "Alias Name" >&2
|
||||
name="$(menu_ask_value "Alias name" "")" || return 0
|
||||
step 1 5 "Alias Name" >&2
|
||||
name="$(menu_ask_value --allow-empty "Alias name" "")" || return 0
|
||||
fi
|
||||
[ -z "$name" ] && { warn "Alias name cannot be empty" >&2; name=""; continue; }
|
||||
if ! _alias_name_valid "$name"; then
|
||||
@@ -380,7 +380,7 @@ _alias_create() {
|
||||
done
|
||||
|
||||
# Step 2: Provider
|
||||
step 2 4 "Provider" >&2
|
||||
step 2 5 "Provider" >&2
|
||||
local pidx
|
||||
pidx="$(_alias_provider_pick)" || return 0
|
||||
local providers=()
|
||||
@@ -407,7 +407,7 @@ _alias_create() {
|
||||
local prompt=""
|
||||
while true; do
|
||||
step 4 5 "System Prompt" >&2
|
||||
prompt="$(menu_ask_value "System prompt (empty = use built-in)" "")" || return 0
|
||||
prompt="$(menu_ask_value --allow-empty "System prompt (empty = use built-in)" "")" || return 0
|
||||
if [[ "$prompt" == *'|'* ]]; then
|
||||
warn "System prompt must not contain '|' characters" >&2
|
||||
prompt=""; continue
|
||||
|
||||
+11
-3
@@ -24,7 +24,7 @@
|
||||
# menu_guard rc 0 iff stdin is a terminal
|
||||
# menu_run <title> <item...> numbered menu loop → chosen index
|
||||
# menu_pick <prompt> <item...> type-to-filter picker → chosen index
|
||||
# menu_ask_value <label> [default] prompted value → entered text
|
||||
# menu_ask_value [--allow-empty] <label> [default] prompted value → entered text
|
||||
# menu_read_value <label> raw-mode bracketed-paste reader
|
||||
# menu_redraw internal redraw (menu_read_value only)
|
||||
|
||||
@@ -347,15 +347,23 @@ menu_redraw() {
|
||||
# text — including multi-line pastes — inserts it literally instead of letting
|
||||
# leftover lines escape to the shell as commands.
|
||||
# rc 0 value on stdout · rc 1 EOF/cancel, or empty answer with no default.
|
||||
# With --allow-empty: empty answer with no default → rc 0 + empty value;
|
||||
# only genuine cancel/EOF returns rc 1.
|
||||
menu_ask_value() {
|
||||
local allow_empty=0
|
||||
if [ "${1:-}" = "--allow-empty" ]; then
|
||||
allow_empty=1
|
||||
shift
|
||||
fi
|
||||
local label="$1" def="${2:-}" val pr="$1"
|
||||
[ -n "$def" ] && pr="$pr [$def]"
|
||||
if ! val="$(menu_read_value "$pr")"; then
|
||||
return 1 # EOF / cancel
|
||||
fi
|
||||
if [ -z "$val" ]; then
|
||||
[ -n "$def" ] || return 1
|
||||
echo "$def"
|
||||
[ -n "$def" ] && { echo "$def"; return 0; }
|
||||
[ "$allow_empty" -eq 1 ] || return 1
|
||||
echo ""
|
||||
return 0
|
||||
fi
|
||||
echo "$val"
|
||||
|
||||
@@ -0,0 +1,129 @@
|
||||
#!/usr/bin/env bash
|
||||
set -euo pipefail
|
||||
# t-menu-allow-empty.sh — regression for the alias-menu fix (2026-09-06):
|
||||
# Part A: menu_ask_value --allow-empty semantics matrix against the REAL
|
||||
# lib/menu-lib.sh via the non-TTY stdin path (the same deterministic
|
||||
# route as the Detective's repro — no TTY, no raw-mode reader).
|
||||
# Default contract (empty + no default → rc 1) must be unchanged;
|
||||
# the flag must flip ONLY the empty+no-default cell (rc 0 + empty);
|
||||
# genuine EOF/cancel must remain rc 1 even with the flag.
|
||||
# Part B: static guards on bin/pos-ai-alias — create-flow steps 1..5 must all
|
||||
# be labeled /5 (FAILS if anyone regresses /4 back), exactly the 2
|
||||
# approved --allow-empty call sites must exist (both in _alias_create),
|
||||
# the edit flow must stay untouched at 4 × /4, and no OTHER tool may
|
||||
# have adopted the flag (scope fence).
|
||||
#
|
||||
# Strategy: production logic is never re-typed. menu_ask_value is sourced from
|
||||
# lib/menu-lib.sh; function bodies are brace-extracted from bin/pos-ai-alias
|
||||
# with the same extract_fn pattern as t-ai-server-validate.sh.
|
||||
|
||||
# extract_fn <source-file> <fnname> — print one brace-delimited function body.
|
||||
extract_fn() {
|
||||
local file="$1" fn="$2"
|
||||
awk -v fn="$fn" '
|
||||
BEGIN { found=0; depth=0 }
|
||||
{
|
||||
if (!found && $0 ~ ("^" fn "\\(\\)")) { found=1; depth=0 }
|
||||
if (found) {
|
||||
n_open = gsub(/\{/, "{")
|
||||
n_close = gsub(/\}/, "}")
|
||||
depth = depth + n_open - n_close
|
||||
print
|
||||
if (depth <= 0) exit
|
||||
}
|
||||
}
|
||||
' "$file"
|
||||
}
|
||||
|
||||
# ask_case <desc> <feed(printf %b bytes)> <expect_rc> <expect_out> <args...>
|
||||
# Feeds literal bytes to the REAL menu_ask_value and asserts rc + stdout.
|
||||
ask_case() {
|
||||
local desc="$1" feed="$2" expect_rc="$3" expect_out="$4"
|
||||
shift 4
|
||||
local out rc
|
||||
set +e
|
||||
out="$(printf '%b' "$feed" | menu_ask_value "$@" 2>/dev/null)"
|
||||
rc=$?
|
||||
set -e
|
||||
check_rc "$desc" "$expect_rc" "$rc"
|
||||
check_eq "$desc (value)" "$expect_out" "$out"
|
||||
}
|
||||
|
||||
run_test() {
|
||||
local sandbox alias_tool
|
||||
sandbox="$(mksandbox menu-allow-empty)"
|
||||
alias_tool="$ROOT/bin/pos-ai-alias"
|
||||
|
||||
source "$ROOT/lib/menu-lib.sh"
|
||||
|
||||
# ═══ Part A0: reader contract the fix builds on (non-TTY fallback) ═══
|
||||
# menu_read_value must distinguish empty-Enter (rc 0 + empty) from EOF (rc 1);
|
||||
# otherwise --allow-empty could never tell cancel from empty.
|
||||
local rr_out rr_rc
|
||||
|
||||
# empty line → rc 0, empty value
|
||||
set +e
|
||||
rr_out="$(printf '\n' | menu_read_value "probe" 2>/dev/null)"; rr_rc=$?
|
||||
set -e
|
||||
check_rc "reader: empty line → rc 0" 0 "$rr_rc"
|
||||
check_eq "reader: empty line → empty value" "" "$rr_out"
|
||||
|
||||
# EOF → rc 1 (cancel)
|
||||
set +e
|
||||
rr_out="$(printf '' | menu_read_value "probe" 2>/dev/null)"; rr_rc=$?
|
||||
set -e
|
||||
check_rc "reader: EOF → rc 1 (cancel)" 1 "$rr_rc"
|
||||
|
||||
# ═══ Part A: --allow-empty semantics matrix (Builder's 7 smoke cases) ═══
|
||||
|
||||
# 1. empty line, no default, WITHOUT flag → rc 1 (default contract unchanged)
|
||||
ask_case "empty-no-default no flag → rc 1 (contract unchanged)" '\n' 1 "" "label" ""
|
||||
# 2. empty line, no default, WITH --allow-empty → rc 0 + empty value
|
||||
ask_case "empty-no-default with flag → rc 0 + empty" '\n' 0 "" "--allow-empty" "label" ""
|
||||
# 3. empty line, default present, WITH flag → rc 0 + default (default wins)
|
||||
ask_case "empty-with-default with flag → default wins" '\n' 0 "def" "--allow-empty" "label" "def"
|
||||
# 4. empty line, default present, no flag → rc 0 + default (regression)
|
||||
ask_case "empty-with-default no flag → default (regression)" '\n' 0 "def" "label" "def"
|
||||
# 5. non-empty line WITH flag → rc 0 + value
|
||||
ask_case "non-empty with flag → rc 0 + value" 'xyz\n' 0 "xyz" "--allow-empty" "label" ""
|
||||
# 6. non-empty line, no flag → rc 0 + value (regression)
|
||||
ask_case "non-empty no flag → rc 0 + value (regression)" 'xyz\n' 0 "xyz" "label" ""
|
||||
# 7. genuine EOF (no input at all) WITH flag → rc 1 (cancel stays cancel)
|
||||
ask_case "EOF with flag → rc 1 (cancel stays cancel)" '' 1 "" "--allow-empty" "label" ""
|
||||
# 8. genuine EOF, no flag → rc 1 (contract, flag independent)
|
||||
ask_case "EOF no flag → rc 1 (contract)" '' 1 "" "label" ""
|
||||
# 9. typed value beats default with flag (default only covers empty)
|
||||
ask_case "typed value beats default with flag" 'xyz\n' 0 "xyz" "--allow-empty" "label" "def"
|
||||
|
||||
# ═══ Part B: static source guards (create-flow step labels + call sites) ═══
|
||||
local create_fn="$sandbox/alias-create-fn.sh"
|
||||
local edit_fn="$sandbox/alias-edit-fn.sh"
|
||||
extract_fn "$alias_tool" _alias_create > "$create_fn"
|
||||
extract_fn "$alias_tool" _alias_edit > "$edit_fn"
|
||||
|
||||
# Create flow: exactly 5 numbered steps, all labeled /5 → FAILS if /4 returns.
|
||||
local nums totals
|
||||
nums="$(grep -oE 'step [0-9]' "$create_fn" | awk '{print $2}' | tr '\n' ' ')"
|
||||
totals="$(grep -oE 'step [0-9] [0-9]' "$create_fn" | awk '{print $3}' | tr '\n' ' ')"
|
||||
check_eq "create flow steps numbered 1..5" "1 2 3 4 5 " "$nums"
|
||||
check_eq "create flow step totals all /5" "5 5 5 5 5 " "$totals"
|
||||
check_eq "create flow has no /4 label" 0 "$(grep -cE 'step [0-9] 4' "$create_fn" || true)"
|
||||
|
||||
# Exactly the 2 approved --allow-empty call sites, both inside _alias_create.
|
||||
check_eq "pos-ai-alias has exactly 2 --allow-empty call sites" 2 \
|
||||
"$(grep -c 'menu_ask_value --allow-empty' "$alias_tool" || true)"
|
||||
check_eq "both --allow-empty sites inside _alias_create" 2 \
|
||||
"$(grep -c 'menu_ask_value --allow-empty' "$create_fn" || true)"
|
||||
|
||||
# Edit flow untouched: still 4 steps, all /4, none with the flag.
|
||||
check_eq "edit flow has 4 numbered steps" 4 \
|
||||
"$(grep -cE 'step [0-9] [0-9]' "$edit_fn" || true)"
|
||||
check_eq "edit flow all /4 (untouched)" 4 \
|
||||
"$(grep -cE 'step [0-9] 4' "$edit_fn" || true)"
|
||||
check_eq "no --allow-empty in edit flow" 0 \
|
||||
"$(grep -c 'menu_ask_value --allow-empty' "$edit_fn" || true)"
|
||||
|
||||
# Scope fence: no OTHER pos tool adopted the flag.
|
||||
check_eq "no other tool adopted --allow-empty (scope fence)" 0 \
|
||||
"$(grep -l 'menu_ask_value --allow-empty' "$ROOT"/bin/pos-* 2>/dev/null | grep -vc 'pos-ai-alias$' || true)"
|
||||
}
|
||||
Reference in New Issue
Block a user