fix: stabilization pass — fail-closed auth, ai flag validation, lint/config/security hardening, regression tests
gates / consistency-and-conventions (push) Successful in 26s
gates / consistency-and-conventions (push) Successful in 26s
17-point code-level audit executed via Explorer->Architect->Builder->Tester->Reviewer;
Reviewer accepted (APPROVE_WITH_NOTES; 3 block-list items resolved):
- security: telegram sender-owner AND-gate + TELEGRAM_OWNER_ID, matrix
MATRIX_ROOM_ID fail-closed, gpg --passphrase-fd 3 (no argv secret),
/dev/tcp positional-arg form (checkport/smb-client/share-lib/NET_PROBE),
eval deny-by-default + --no-command-execution carried by both chat bridges,
tty-gated --trust; config/{telegram,matrix}.env reference templates
- ai: all ExecStart flags validated against installed llama.cpp
(requested->error, default->omit+warn, CONFIG_REQUESTED_FLAGS); single-file
hf download failure rc=1 + no .hf-meta; LLAMACPP_HOST coherent;
POS_SUBCMDS + metadata gaps closed
- tooling: lint-conventions Bash-native rewrite (~24-30x faster, rules and
output byte-identical, :num restored); pos system uninstall covers all 12
libs + scale-tail + flags dir + systemd user units (|| true) + plugin
markers; anchored .bash_completion/.bashrc removal replaces 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 unmigrated (source semantics)
- tests: first committed regression suite — tests/run-tests.sh zero-dep
runner + make test; 12 files / 179 checks / 0 skip / ~52s; hard skip
contract; systemd-analyze verify on generated unit PASS
Verified: make gen idempotent; make check green; make lint 0 FAIL, 0 WARN;
make test green; bash -n clean; git diff --check clean. Audit deliverables +
agent reports + AGENT_TODO Done entry included.
This commit is contained in:
Your Name
@@ -0,0 +1,136 @@
|
||||
# Tester Report — Matrix-auth daemon-hang rc assertion (Reviewer N1) — 2026-09-06
|
||||
|
||||
## TL;DR
|
||||
|
||||
- **Status: TESTS_READY** — regression-suite hardening for Reviewer `AgentsReport/reviewer/2026-09-06_stab_acceptance.md` Note N1: `t-matrix-auth.sh` run 1 lacked an exit-code assertion, so a daemon-hang regression could pass vacuously.
|
||||
- **Defects added:** 2 new `check_rc` assertions (run 1 authorized + run 2 room-unset). File `t-matrix-auth.sh`: 8 → **10 checks**.
|
||||
- **Suite:** `make test` → **12 files pass / 0 fail / 0 skip**, **179 checks pass**, runtime **45s**, exit 0. No flakes observed.
|
||||
- **Production code: untouched.** Only `tests/t-matrix-auth.sh` modified; `tests/test-lib.sh` unchanged (the `check_rc` helper already exists).
|
||||
- **Key correction to the brief:** asserting `TR_RC` under plain `timeout` is vacuous — GNU `timeout` reports **124 in BOTH** the healthy and broken-trap cases. The discriminating fix is `timeout --preserve-status -k 2 …` + assert the daemon's own TERM-trap exit status.
|
||||
|
||||
---
|
||||
|
||||
## Step 1: Confirm the gap (Reviewer N1)
|
||||
|
||||
Reviewed `tests/t-matrix-auth.sh` run 1: `test_run_env … -- timeout 5 "$listener" --run` with `check_contains`/`check_file_exists`/`check_eq` but **no rc assertion**. Confirmed the reviewer's concern: a broken TERM trap (daemon ignores TERM) would not fail any existing check.
|
||||
|
||||
Also confirmed the deeper problem with the naive fix: plain `timeout 5` returns **124 whether or not the TERM trap works** (verified empirically — both the working daemon and a no-trap daemon yield 124). So `check_rc … 124` would be itself vacuous.
|
||||
|
||||
Evidence (throwaway probes, `/tmp/opencode`, removed after):
|
||||
```
|
||||
timeout 1 bash -c 'trap "exit 0" TERM; sleep 30' → rc=124 (trap WORKS)
|
||||
timeout 1 bash -c 'sleep 30' → rc=124 (no trap)
|
||||
timeout --preserve-status 1 … (trap WORKS) → rc=0
|
||||
timeout --preserve-status 1 … (no trap) → rc=143 (SIGTERM)
|
||||
```
|
||||
|
||||
[PASS]
|
||||
|
||||
## Step 2: Design the discriminating assertion
|
||||
|
||||
Goal per the brief: *"so the test genuinely fails on a daemon-hang regression"*. The daemon is an infinite polling loop that only terminates via its `TERM` trap (`trap 'kill $(jobs -p) 2>/dev/null; exit 0' TERM INT`), so `timeout` must always be the one signalling it. To make the daemon's own exit status observable:
|
||||
|
||||
- **`--preserve-status`** — `timeout` forwards the child's real exit status instead of forcing 124.
|
||||
- **`-k 2` (`--kill-after`)** — bounds the wait: if a broken trap ignores TERM, `timeout` SIGKILLs at +2s so the test cannot hang the whole suite indefinitely.
|
||||
|
||||
**Empirical healthy-path exit status (current production code):** the listener's TERM trap is `kill $(jobs -p) 2>/dev/null; exit 0`. Under `set -euo pipefail`, with no background jobs `kill` (no args) fails with **rc 2**, which triggers errexit **before** the `exit 0` and aborts the trap → the daemon actually exits with **rc 2** (verified on the real listener and a minimal `set -euo pipefail` repro: `rc=2`, the post-kill `exit 0` never runs).
|
||||
|
||||
- A genuine **daemon-hang** (TERM ignored / trap non-exiting): `--kill-after` SIGKILLs → **rc 137**.
|
||||
- A healthy daemon: TERM trap fires → **rc 2**.
|
||||
|
||||
So `check_rc … 2` meaningfully discriminates: healthy = daemon's own trap exit (2, definitively not a forced kill), regression = 137/124.
|
||||
|
||||
**Note (observation for Orchestrator, NOT fixed — out of scope):** the `kill $(jobs -p)` in the daemon's TERM trap fails under errexit, so the daemon exits 2 rather than the intended 0. Harmless to the daemon's operation (it still terminates, no hang) but the `exit 0` is effectively dead. Flagged for a possible future Builder fix; intentionally not addressed here (production code out of scope). The rc assert documents current healthy behavior and still catches a hang.
|
||||
|
||||
[PASS]
|
||||
|
||||
## Step 3: Change — `tests/t-matrix-auth.sh`
|
||||
|
||||
- Run 1 (authorized): added `--preserve-status -k 2` and `check_rc "daemon terminated via TERM trap, not killed (no hang)" 2 "$TR_RC"`.
|
||||
- Run 2 (room-unset): same `--preserve-status -k 2` + `check_rc … 2`.
|
||||
- Comment explains why `--preserve-status -k 2` + rc 2 catches the regression and why plain `timeout` would be vacuous.
|
||||
|
||||
Snippet (run 1):
|
||||
```bash
|
||||
# --preserve-status + --kill-after surface the daemon's own TERM-trap exit,
|
||||
# so a broken trap (daemon-hang regression → SIGKILL 137 / timeout 124)
|
||||
# genuinely fails the rc assert instead of passing vacuously. --kill-after
|
||||
# also bounds the wait so a hung daemon can't stall the whole suite.
|
||||
test_run_env "${common[@]}" -- timeout --preserve-status -k 2 5 "$listener" --run
|
||||
check_rc "daemon terminated via TERM trap, not killed (no hang)" 2 "$TR_RC"
|
||||
```
|
||||
|
||||
Run 2 mirrors it with the room-unset env (no `MATRIX_ROOM_ID`) and desc `"room-unset daemon terminated via TERM trap, not killed (no hang)"`.
|
||||
|
||||
`tests/test-lib.sh` was not modified — `check_rc` already exists (`test-lib.sh:19`).
|
||||
|
||||
[PASS]
|
||||
|
||||
## Step 4: Mutation probe — the new assert genuinely fails on a hang
|
||||
|
||||
Copied the listener into a scratch tree and replaced the TERM trap with a non-exiting handler (`trap 'hang…sleep 30' TERM INT`) to simulate the daemon-hang regression, then ran it under the same `timeout --preserve-status -k 2` invocation:
|
||||
|
||||
```
|
||||
… timeout --preserve-status -k 2 3 "$listener" --run
|
||||
ignoring TERM (regression), sleeping
|
||||
rc=137
|
||||
```
|
||||
|
||||
Expected `2`, actual `137` → the `check_rc 2` assert **rejects** the regression and the test fails. Healthy path verified → `rc=2` → assert passes. Scratch cleanup performed; no production file touched.
|
||||
|
||||
[PASS]
|
||||
|
||||
## Step 5: Full suite — `make test`
|
||||
|
||||
```
|
||||
Running 12 test file(s) — strict mode: no network, no sudo, no system changes.
|
||||
PASS t-ai-hf-download.sh (10 checks)
|
||||
PASS t-ai-llama-detect.sh ( 9 checks)
|
||||
PASS t-ai-server-flags.sh (28 checks)
|
||||
PASS t-config-precedence.sh (43 checks)
|
||||
PASS t-gen-docs-drift.sh ( 4 checks)
|
||||
PASS t-gpg-password.sh (14 checks)
|
||||
PASS t-lint-gate.sh ( 5 checks)
|
||||
PASS t-matrix-auth.sh (10 checks)
|
||||
PASS t-systemd-unit.sh (11 checks)
|
||||
PASS t-telegram-auth.sh ( 8 checks)
|
||||
PASS t-uninstall-manifest.sh(18 checks)
|
||||
PASS t-unsupported-flags.sh (19 checks)
|
||||
──────────────────────────────────────────────
|
||||
Summary: files 12 pass / 0 fail / 0 skip (of 12)
|
||||
Checks : 179 pass / 0 fail / 0 skip
|
||||
Runtime: 45s
|
||||
```
|
||||
|
||||
Exit 0. `t-matrix-auth.sh` independently re-run at 10 checks (10s). No flake source observed; isolated result is representative (no mid-edit contention observed on the suite path — `bin/pos`/`bin/pos-*` are only read by the tests, never written here).
|
||||
|
||||
[PASS]
|
||||
|
||||
---
|
||||
|
||||
## Verification completed
|
||||
|
||||
- Baseline `t-matrix-auth.sh` (8 checks, 10s) before edit — green.
|
||||
- Post-edit standalone run (10 checks, 10s) — green.
|
||||
- Mutation probe proves the new assert fails (rc 137) on a broken TERM trap / daemon hang.
|
||||
- Full `make test` green: 179/179 checks, 45s.
|
||||
- `test-lib.sh` untouched; no production code touched; no commits made.
|
||||
|
||||
## Coverage note
|
||||
|
||||
The new asserts close the reviewer's vacuous-pass gap: a daemon-hang (TERM-trap) regression now produces rc 137/124 and fails the test rather than passing silently. Both authz runs (authorized + fail-closed) carry the guard.
|
||||
|
||||
## Remaining uncertainty / out of scope
|
||||
|
||||
- The daemon's dormant `exit 0` in its TERM trap (exits 2 under errexit) — see Step 2 observation. Functional no-hang is preserved; left for Builder/Architect if they want the trap to truly exit 0.
|
||||
- Not committed (per brief).
|
||||
|
||||
## Recommended next agent
|
||||
|
||||
**Reviewer** — the regression-suite gap is closed and green; suitable for independent verification of the rc assert.
|
||||
|
||||
**Reason:** Tester completes measurement/verification; the change is in test-only scope, ready for reviewer sign-off.
|
||||
|
||||
## Changes made by Tester
|
||||
|
||||
- `tests/t-matrix-auth.sh` — added `--preserve-status -k 2` to both `timeout` invocations and two `check_rc … 2` (no-hang) assertions; explanatory comments. No other files touched, no commits.
|
||||
@@ -0,0 +1,93 @@
|
||||
# Tester Report — Regression Test Infrastructure + First Suite (2026-09-06)
|
||||
|
||||
## TL;DR (updated continuously)
|
||||
|
||||
- **Status:** TESTS_READY — 12/12 test files pass, 177 checks, runtime ~46s (`make test`).
|
||||
- **Deliverables:** `tests/run-tests.sh` (zero-dep runner), `make test` target, 12 `tests/t-*.sh` files, `tests/README.md`.
|
||||
- **Findings (production bugs discovered):** none — no production bug surfaced; all defects found during test iteration were in the test framework/stubs/test assertions themselves (see Step 4).
|
||||
- **Suite timing / counts:** `make test` (2026-09-06): files **12 pass / 0 fail / 0 skip**, checks **177 pass / 0 fail / 0 skip**, runtime **46s** (44s on rerun); exit 0.
|
||||
- **Gates:** `make lint` still `0 FAIL, 0 WARN`; `make check` now PASS (parallel-track gen drift resolved upstream during this session); tests/ has zero lint/check surface.
|
||||
|
||||
---
|
||||
|
||||
## Step 1: Environment baseline (before adding tests)
|
||||
|
||||
- `make check` at start: **FAILED** — `doc/code drift` (expected: parallel Builder tracks have uncommitted changes; gen output in the working tree not yet refreshed). Now resolves to PASS after upstream sync.
|
||||
- `make lint` at start: **PASS** — `0 FAIL, 0 WARN` (3.5s).
|
||||
- `make gen` idempotence on a pristine temp copy: **PASS** (2× ~1.4–1.8s; `git status --porcelain` empty after 2nd gen).
|
||||
- `systemd-analyze verify` prototype: passes (rc 0) when ExecStart binary exists and model path is quoted.
|
||||
- Config-loader migration (D-D): **landed in the working tree** — `load_env_file` present in `lib/config-ui.sh:336`; all 9 tools call it. Config-precedence tests target the final contract.
|
||||
|
||||
## Step 2: Framework + suite files (status below)
|
||||
|
||||
- [x] `tests/run-tests.sh`
|
||||
- [x] `tests/test-lib.sh`
|
||||
- [x] `tests/t-ai-server-flags.sh`
|
||||
- [x] `tests/t-ai-hf-download.sh`
|
||||
- [x] `tests/t-ai-llama-detect.sh`
|
||||
- [x] `tests/t-unsupported-flags.sh`
|
||||
- [x] `tests/t-systemd-unit.sh`
|
||||
- [x] `tests/t-telegram-auth.sh`
|
||||
- [x] `tests/t-matrix-auth.sh`
|
||||
- [x] `tests/t-gpg-password.sh`
|
||||
- [x] `tests/t-config-precedence.sh`
|
||||
- [x] `tests/t-uninstall-manifest.sh`
|
||||
- [x] `tests/t-gen-docs-drift.sh`
|
||||
- [x] `tests/t-lint-gate.sh`
|
||||
- [x] `Makefile` `test:` target
|
||||
- [x] `tests/README.md`
|
||||
|
||||
## Step 3: Full suite run (final)
|
||||
|
||||
Command: `make test` (target: `./tests/run-tests.sh`) — 2026-09-06.
|
||||
|
||||
```
|
||||
Running 12 test file(s) — strict mode: no network, no sudo, no system changes.
|
||||
|
||||
PASS t-ai-hf-download.sh (10 checks)
|
||||
PASS t-ai-llama-detect.sh (9 checks)
|
||||
PASS t-ai-server-flags.sh (28 checks)
|
||||
PASS t-config-precedence.sh (43 checks)
|
||||
PASS t-gen-docs-drift.sh (4 checks)
|
||||
PASS t-gpg-password.sh (14 checks)
|
||||
PASS t-lint-gate.sh (5 checks)
|
||||
PASS t-matrix-auth.sh (8 checks)
|
||||
PASS t-systemd-unit.sh (11 checks)
|
||||
PASS t-telegram-auth.sh (8 checks)
|
||||
PASS t-uninstall-manifest.sh (18 checks)
|
||||
PASS t-unsupported-flags.sh (19 checks)
|
||||
|
||||
──────────────────────────────────────────────
|
||||
Summary: files 12 pass / 0 fail / 0 skip (of 12)
|
||||
Checks : 177 pass / 0 fail / 0 skip
|
||||
Runtime: 46s
|
||||
```
|
||||
|
||||
Exit code 0. Rerun via `make test`: files 12/12 pass, 44s. `make lint` unaffected (`0 FAIL, 0 WARN`), `make check` passes (parallel drift resolved upstream, not by this track).
|
||||
|
||||
[PASS]
|
||||
|
||||
## Step 4: Defects found and fixed during test iteration (all in test artifacts, none in production)
|
||||
|
||||
1. `tests/test-lib.sh` `check_rc` — `$desc` read before `local desc="$1"` declaration → `set -u` crash on first use. Fixed.
|
||||
2. `tests/run-tests.sh` — `set -e` in the runner killed the PARENT when a test subshell exited nonzero (e.g. test 4 aborted after 3 passing tests). Fixed: subshell wrapped in `if (…); then rc=0; else rc=$?; fi`; verified a failing test now records FAIL and continues. Also: bare-name args (`run-tests.sh t-gpg-password`) now resolve `$TEST_DIR/<name>.sh`.
|
||||
3. `tests/t-ai-hf-download.sh` stub — embedded JSON via `$(cat "$tree_resp")` broke stub quoting → replaced with `cat "$TREE_RESP"` env passthrough; `for (( ; i<=$#; i++ ))` expanded `$#` at stub-write time → escaped `\$#`; `base_env` typo → `env_base`; `return 1` at stub top level → `exit 1` (see #6).
|
||||
4. `tests/t-ai-llama-detect.sh` — asserted literal `cpu`; tool emits `gpu: CPU` (case differs) → assertions corrected to actual token shape.
|
||||
5. `tests/t-config-precedence.sh` — Part A env-wins probe `FOO=envval load_env_file …` evaluated in the PARENT shell (no persistence) → rewrote as explicit subprocess with `export` + captured output; Part B needed llama-server + nvidia-smi stubs for the deps guard; Part D legacy guard was a false positive — exactly 3 documented `load_system_env` callers (pos-media-sync, pos-system-backup, pos-system-health) → whitelist those and assert count == 3.
|
||||
6. **Stub scripts: `return` at top level of a non-sourced script is an ERROR in bash and falls through** (`return: can only 'return' from a function or sourced script`), so every stub response silently gained a trailing `{"ok":true}` → corrupt JSON → listeners slept in a 5s retry loop and never processed (`jq -r '.ok'` returned `true\ntrue`). Fixed all stub heredocs to `exit 0` (telegram/matrix curl stubs; ai-hf already used `exit`).
|
||||
7. `tests/t-telegram-auth.sh` / `tests/t-matrix-auth.sh` — two line-continuation bugs in `test_run_env` invocations: a missing trailing `\` meant the env-var list became a separate command and `test_run_env` ran bare `env` (prints the whole environment — the mysterious `SHELL=/bin/bash` output) with rc 0. Fixed by single-line invocation. Matrix reply count needle `m.room.message` also matched the URL-encoded sync filter on every `/sync` line → narrowed to `/send/m.room.message`.
|
||||
8. `tests/t-gpg-password.sh` — artifact-leftover checks false-failed because run 1's `.gpg` remained on disk for runs 2/3 → now `rm -rf "$work"; mkdir` between runs; bare `--passphrase` guard now token-exact (`grep -c '^--passphrase$'`) since `--passphrase-fd` legitimately contains the substring.
|
||||
9. `tests/t-lint-gate.sh` — negative case invoked the REAL lint (absolute path); `lint-conventions.sh` computes `ROOT="$(dirname "$0")/.."` and `cd`s THERE, so it linted the real repo (clean), not the planted copy. Fixed: run the copy's own `scripts/lint-conventions.sh` (relative path) from inside the copy.
|
||||
10. `tests/t-uninstall-manifest.sh` — POS_LIBS extraction awk `<^POS_LIBS=( … {getline; while(1)…}` never matched a lone `^)` line because the block is `POS_LIBS=(… \⏎ …registry.sh)` (two lines, `)` on the second) → getline at EOF returns 0, loop spins forever at EOF → the whole test hung (this was the full-suite 300s hang). Replaced with a sed range `/^POS_LIBS=(/,/)$/p` + normalization; also the leftover-gap whitespace made the sorted diff fail (collapsed with `tr -s`), and plugin-removal marker check now greps `POS_PLUGIN` (the marker `installed_plugins()` scans for) instead of a literal `^# POS_PLUGIN:` in the uninstall script.
|
||||
11. `tests/t-systemd-unit.sh` — systemd unit uses double quotes (not backslash escaping) for the model path → assertion corrected; `EnvironmentFile` check compared against the unit PATH instead of its content → `$(cat "$unit")`.
|
||||
12. `tests/t-unsupported-flags.sh` — real error text is `installed llama.cpp <v> does not expose <flag> — remove it or upgrade llama.cpp`, not "does not support" → assertions updated.
|
||||
|
||||
None of the above touched production code. `make check` / `make lint` / `make gen` results are unchanged by this track (verify with `make check && make lint` — both currently green).
|
||||
|
||||
## Step 5: Coverage notes & handoff
|
||||
|
||||
- **Behavior covered per area:** ai-server flag seam (CLI/config/env/default precedence + unsupported-flag hard error + dedupe) 28; config file precedence + legacy loaders 43; gpg password hygiene (fd-only, no bare token, no secret in argv, artifact cleanup on enc/verify failure) 14; systemd unit generation (ExecStart quoting, environment/deps/secrets lines, `systemd-analyze verify`) 11; telegram/matrix authz fail-closed gates 8+8; ai hf download stub network behavior 10; llama detection stub 9; gen/lint gates (positive + planted-violation negative) 4+5; uninstall manifest symmetry + XDG scan tier + POS_PLUGIN marker 18; unsupported-flag matrix 19.
|
||||
- **What is not covered (deliberately):** real network/sudo/docker paths (stubbed only); `pos entertainment send` live-plugin e2e (requires Telegram token); anything requiring root. These are outside the sandbox contract of this suite and remain manual checks.
|
||||
- **Suite hygiene:** deterministic sorted order, per-test sandbox auto-clean, per-file logs, no network/sudo/system mutations, skip contract, total < 90s.
|
||||
|
||||
[PASS]
|
||||
Reference in New Issue
Block a user