[WIP] Review Fixes, Stabilty and live testing results

ISSUES.md

@@ -8,7 +8,7 @@ See `PLAN.md` for the full phased execution plan derived from this log.
 
 ---
 
-## Completed work (2026-04-22, round 5 — PR #4 Copilot review fixes)
+## Completed work (2026-04-21, round 5 — PR #4 Copilot review fixes)
 
 ### PR #4 Copilot inline review comments resolved ✓
 
@@ -31,7 +31,7 @@ See `PLAN.md` for the full phased execution plan derived from this log.
 
 ---
 
-## Completed work (2026-04-22, round 4 — upstream carry-forward sweep)
+## Completed work (2026-04-21, round 4 — upstream carry-forward sweep)
 
 ### Upstream tcsh-org/tcsh bug fixes applied ✓
 
@@ -83,15 +83,14 @@ Items **not applied** (rejected upstream or out of scope):
 Items **still open upstream and tracked in mcsh** (see "Remaining open items"):
 
 - **#119** — `unshare --user --pid` hang (critical)
-- **#117 / #121** — Unicode/wide-char regression (critical)
 - **#93** — `ls-F` colour with `CLICOLOR_FORCE` (low)
 - **#102 / #82** — Acute accent lintian; man page pipe workaround (low)
 - **#123** — Syntax improvement/alias multi-line (feature request; tracking)
 - **#113** — Redirect in `{ }` expression blocks (resolved in mcsh, Phase 5)
 
 ---
 
-## Completed work (2026-04-22, round 3 — PR3 CodeRabbit round-2 review fixes)
+## Completed work (2026-04-21, round 3 — PR3 CodeRabbit round-2 review fixes)
 
 ### Phase 8 (round 3) — CodeRabbit PR3 review fixes ✓
 
@@ -351,9 +350,6 @@ integration (no raw ESC bypass).
 - **#119** (`sh.proc.c`) — `unshare --user --pid` hang. Fork retry loop sleeps
   with interrupts disabled. Fix: use `SIGALRM`-based timeout or `nanosleep`
   with signal unblocking.
-- **#117 / #121** (`sh.lex.c`, `sh.dol.c`) — Unicode regression: emoji/wide
-  chars stripped from filenames and variable assignments since 6.24.14. Root
-  cause: byte vs. character length confusion in the wide-string path.
 - **#93** (`tw.color.c`) — `ls-F` colour failures with `CLICOLOR_FORCE`,
   `LSCOLORS`, `LS_COLORS`. Audit colour detection and environment-variable
   precedence.
@@ -367,12 +363,6 @@ integration (no raw ESC bypass).
   wide-character input or terminal resize. Full fix: integrate ghost rendering
   into the `Refresh()` pipeline.
 
-### 4. Test suite
-
-- `tests/` not yet initialised. Minimum suite required: startup file order,
-  `$mcsh`/`$tcsh` variable correctness, unicode filename round-trip, expression
-  overflow, job-count prompt, `cd -N` stack navigation.
-
 ### 5. Scope of this consolidation push
 
 Present on the branch:
@@ -392,6 +382,198 @@ Present on the branch:
 Explicitly deferred / excluded:
 
 - **Native Windows support** — dropped.
-- **Test suite (`tests/`)** — deferred.
 - **Autogenerated `configure` script** — not committed; regenerate with
   `autoreconf -fi`.
+
+---
+
+## Round 6 — Review response: three flagged weaknesses (dev4) (2026-04-21)
+
+Addresses all findings from the deep-dive analysis (paste_1 / paste_2) and
+Gemini PR #5 inline comments.
+
+### 1. Short-circuit evaluation (`sh.dol.c`)
+
+**Root cause (confirmed):** `Dfix()` in `sh.sem.c` expands all `$` tokens
+*before* `doif` calls `expr()`. The expression evaluator in `sh.exp.c` already
+implements correct `TEXP_IGNORE` short-circuit at the `exp0`/`exp1` level, but
+`Dfix` runs unconditionally before evaluation, so `"$a"` in
+`if ($?a && "$a" != "")` threw `ERR_UNDVAR` before `&&` could suppress it.
+
+**Fix (`sh.dol.c:643`):** In `Dgetdol()`, when a variable is unset and not
+found in the environment, instead of calling `udvar()` (which throws
+`ERR_UNDVAR`), set `dolp = STRNULL` and jump to `eatbrac`. This makes unset
+`$varname` silently expand to `""` — matching bash/zsh double-quote semantics
+— so the expression evaluator receives `"" != ""` (false) rather than dying.
+`$?varname` continues to work correctly via the existing `bitset` path.
+
+**Test:** `t003_shortcircuit.sh` — `unset a; if ($?a && "$a" != "") echo yes`
+must produce no output and exit 0.
+
+### 2. Unicode regression (`sh.lex.c`, `sh.dol.c`) — fixed
+
+**Scope:** Inherited from tcsh 6.24.14. Byte-vs-character length confusion in
+the wide-string expansion path caused multi-byte characters (emoji, CJK, Latin
+Extended) to be dropped or corrupted during filename glob expansion and variable
+assignment. Affected any locale where `MB_CUR_MAX > 1`.
+
+**Affected upstream issues:** tcsh #117, #121.
+
+**Root cause:** Two `mbtowc` accumulation loops compared the partial-byte count
+against `MB_LEN_MAX` (compile-time worst case across all locales, 16 on glibc)
+instead of `MB_CUR_MAX` (runtime maximum for the current locale, 4 for UTF-8).
+When `mbtowc` returned `-1` for a stray invalid byte, the loop continued reading
+up to 15 additional bytes of lookahead before giving up, swallowing valid
+multi-byte sequences that immediately followed.
+
+**Fix (two-line change):**
+- `sh.lex.c` `wide_read()` — `(partial - i) < MB_LEN_MAX` → `(partial - i) <
+  (size_t)MB_CUR_MAX`. Covers script-file reads, stdin pipes, and backquote
+  command substitution.
+- `sh.dol.c` `Dgetdol()` `$<` accumulation loop — `cbp < MB_LEN_MAX` → `cbp <
+  (size_t)MB_CUR_MAX`. Covers the `$<` line-read primitive.
+
+The corrected pattern matches the existing reference implementation at
+`ed.inputl.c:814`. Buffer declarations (`char cbuf[MB_LEN_MAX]`) are unchanged
+because they must size for the worst case across all platforms.
+
+**Tests:** `tests/t009_unicode_vars.sh` through `tests/t014_unicode_script_source.sh`
+cover variable round-trip, `$%` character count, glob expansion, `$<` stdin read,
+backquote substitution, invalid-byte recovery, and sourced-script Unicode.
+
+### 3. Test suite — initial suite created (`tests/`)
+
+`tests/` directory created with 8 regression scripts and a `Makefile`:
+
+| Script | What it tests |
+|--------|---------------|
+| `t001_vars.sh` | `$mcsh` and `$tcsh` are set on startup |
+| `t002_overflow.sh` | `@ x = (1 << 31)` yields `2147483648` (unsigned left-shift) |
+| `t003_shortcircuit.sh` | `$?a && "$a" != ""` is silent when `$a` unset |
+| `t004_pipe_to_var.sh` | `echo foo \| set x` assigns `x=foo` |
+| `t005_cd_stack.sh` | `pushd`/`cd -1` navigates directory stack correctly |
+| `t006_function_builtin.sh` | `function` builtin stores and executes body |
+| `t007_arith_rsh.sh` | `@ x = (-8 >> 1)` yields `-4` (signed right-shift) |
+| `t008_unset_modifiers.sh` | `${unset:h}` and `$#unset` don't error when var is unset |
+
+Run with: `make -C tests MCSH=./mcsh check`
+
+### 4. Gemini PR #5 inline comment — `cache_store()` goto removed (`ed.syntax.c`)
+
+Rewrote `cache_store()` with two explicit loops: first pass finds an empty
+slot; second pass (only if needed) scans for the LRU victim. No `goto`. Logic
+and semantics are identical; the victim variable is initialised to `-1` so the
+first pass's early-break is the only way it gets set to a valid index before
+the second pass.
+
+### 5. Gemini PR #5 inline comment — magic number `2` replaced (`tc.prompt.c`)
+
+Added `#define GIT_POLL_INTERVAL 2` near the top of the file and replaced the
+literal `2` in the throttle check with the named constant.
+
+---
+
+## Round 7 — PR #5 Copilot + Gemini review response (2026-04-21)
+
+### 1. `sh.dol.c` — unset variable modifier handling fixed
+
+**Copilot + Gemini finding:** The unset-variable expansion path jumped directly to
+`eatbrac` without calling `fixDolMod()`, causing `${unset:h}` and similar
+modifier expressions to crash with "Missing }" because the `:h` was left in
+the input stream. Also, `$#unset` (dimen) and `$%unset` (length) did not return
+a sensible value.
+
+**Fix:** Call `fixDolMod()` before branching to `eatbrac`, consume modifiers
+properly, and return `0` for both `$#unset` and `$%unset` (consistent with
+treating an unset variable as empty/zero-length).  The comment now correctly
+states this applies to all variable expansions, not only double-quoted ones.
+
+**Test:** `t008_unset_modifiers.sh` — `${unset:h}` must not error; `$#unset` must yield `0`.
+
+### 2. `tests/run_tests.sh` — portability hardening
+
+**Copilot findings:**
+- Header comment incorrectly stated scripts "print PASS or FAIL"; they actually
+  exit 0/non-zero with optional failure output.
+- Glob `t*.sh` could iterate the literal pattern on a `/bin/sh` with no
+  matching files; now guarded with `set -- t*.sh; [ -e "$1" ] || exit`.
+- `echo "$result"` with arbitrary content is non-portable (leading `-n` or
+  backslash sequences); replaced with `printf '%s\n' "$result"` throughout.
+
+### 3. `tests/t006_function_builtin.sh` — mktemp portability
+
+**Copilot finding:** `mktemp /tmp/t006.XXXXXX.csh` fails on BSD/macOS because
+`mktemp` requires the template to end with X characters (suffixes after the X
+block are rejected). Removed `.csh` suffix — the shell interpreter is set by
+the heredoc content, not the filename.
+
+### 4. `tests/Makefile` — was already present
+
+The `tests/Makefile` was created in Round 6 and supports `make check` and
+`make MCSH=/path/to/mcsh check`. All documentation references to
+`make -C tests MCSH=./mcsh check` are therefore accurate.
+
+### 5. `ed.syntax.c` — syntax highlighting improvements
+
+- Fixed `in_table()`: removed unused loop variable `i` (loop is pointer-based).
+- Fixed `ST_VARIABLE` state machine: `$$`, `$!`, `$<` are single-character
+  special variables and now correctly transition to `ST_NORMAL` after being
+  coloured.  Previously the redundant inner check re-tested `buf[i]` (same as
+  `ch`) causing `$$` to sometimes stay in variable state.
+- Extended redirection operator colouring to cover `>!`, `>>!`, `>|`, `>>&`
+  (noclobber-override and append-noclobber forms).
+
+### 6. `ed.chared.c` — command- and file-aware predictive autocomplete
+
+`predict_from_history()` now falls through to two additional predictors when
+no history match is found:
+
+- **`predict_file()`** — fires when the current word starts with `/`, `./`, or
+  `~/`.  Splits the word into directory + basename prefix, does a single
+  `opendir()` scan, and sets GhostBuf to the unique suffix.  Directories get a
+  trailing `/`.  Ambiguous or no matches produce no ghost.
+- **`predict_cmd()`** — fires when the word is at the command position in the
+  input line (no prior non-space characters, or immediately after `;`/`|`/`&`).
+  Scans all `$PATH` directories for a uniquely matching executable and sets
+  GhostBuf to the suffix.
+
+Priority: history > file path > command name.  Existing Tab completion is
+unchanged; the new predictors are ghost-text only (accept with right-arrow /
+Ctrl-F).
+
+### 7. `tests/t008_unset_modifiers.sh` — new regression test
+
+Covers the `${unset:h}` modifier fix and `$#unset` == 0 behaviour.
+
+---
+
+## Round 9 — PR #5 review response part 2 (Apr 2026)
+
+### 1. `ed.syntax.c` — redirection coloring tightened
+
+**CodeRabbit finding:** Redirection continuation loop was over-broad for `<`.
+Fixed to only allow `!` and `|` when the opening operator is `>`. Both still
+accept `&`, `-`, `>`, and `<`.
+
+### 2. `ed.chared.c` — predictive completion enhancements
+
+- **Caching:** Added caching for `predict_file()` and `predict_cmd()` to avoid
+  redundant filesystem scans on every keystroke. `f_cache` tracks directory
+  mtime; `c_cache` tracks the `$PATH` string.
+- **User Toggle:** All predictive logic gated behind `set predict` shell
+  variable.
+- **`~user` Expansion:** `predict_file()` now supports `~user/` expansion via
+  `getpwnam()`.
+- **Empty PATH components:** `predict_cmd()` now treats empty components in
+  `$PATH` as the current directory (`.`), matching `cmd_on_path()`.
+
+### 3. Test suite hardening
+
+- **`tests/run_tests.sh`:** Now recognizes exit code `77` as `SKIP` and reports
+  it in the summary. Fixed a literal newline in an error message.
+- **`tests/lib_locale.sh`:** Updated to use portable ERE (`grep -E`) and exit
+  code `77` for skips.
+- **`tests/t006_function_builtin.sh`:** Added cleanup `trap` and exit status
+  verification.
+- **`tests/t008_unset_modifiers.sh`:** Escaped `$` in failure message and
+  switched to portable `grep -E`.

PLAN.md

@@ -227,7 +227,7 @@ Features developed natively for mcsh, with no upstream tcsh counterpart.
 
 | Feature | `set` variable | Primary Files | Notes |
 |---------|----------------|---------------|-------|
-| Fish-style predictive autocomplete | *(always active)* | `ed.chared.c`, `ed.refresh.c`, `ed.inputl.c` | Scans `Histlist` for prefix match; ghost text rendered dimmed after cursor. Right-Arrow / `^F` accepts. |
+| Fish-style predictive autocomplete | `set predict` | `ed.chared.c`, `ed.refresh.c`, `ed.inputl.c` | Predicts file paths (`predict_file`) and commands (`predict_cmd`), controllable via the `set predict` toggle. Ghost text rendered dimmed after cursor. Right-Arrow / `^F` accepts. |
 | Native git branch prompt escapes | *(always active)* | `tc.prompt.c` | `%g` = branch name; `%G` = branch + operation state. Cached per-CWD with independent HEAD and state-marker mtime tracking. |
 | Filetype colouring in completion | `set color` | `tw.color.c`, `sh.set.c` | Drives `ls-F` completion listings via `LSCOLORS`/`LS_COLORS`. |
 | **Interactive syntax highlighting** | **`set syntax`** | **`ed.syntax.c/h`, `ed.screen.c`, `ed.refresh.c`, `ed.inputl.c`, `sh.set.c`** | Virtual-display pipeline integration. Single-pass tokeniser fills `SyntaxColor[]`; `Draw()` propagates token colour into `Vdisplay[]` via `SYN_PACK()`; `so_write()` emits ANSI SGR per cell via `SetSGRColor()`. 32-entry LRU command cache avoids per-keystroke `stat(2)`. |
@@ -276,4 +276,7 @@ Key fixes:
 | 2026-04-21 | Phase 4b + Phase 8 (round 1): all Gemini + CodeRabbit PR3 review items addressed. `vms.termcap.c` repurposed as portable termcap shim; `sh.func.c` `doif` widened to `tcsh_number_t`; `configure.ac` fixes; `dot.mcshrc` rewritten. README, PLAN, ISSUES updated. |
 | 2026-04-21 | Phase 9: native interactive syntax highlighting (`set syntax`) landed. Virtual-display pipeline: `ed.syntax.c/h` tokeniser + LRU cache; `SYN_PACK`/`SYN_TOK`/`SYN_GLYPH` bit-packing into `Vdisplay Char`; `SetSGRColor()` per-cell ANSI SGR. |
 | 2026-04-21 | Phase 8 (rounds 2–3): remaining Copilot review findings resolved — `TCSH_BASELINE_VERSION` string literal; git cache marker-mtime independence; `SetSGRColor`/`DrawGhost` `ESC[22;39m` SGR fix; `ed.inputl.c` no double `Refresh()`; zsh-style pushd/popd tree display and `cd -N` navigation added. All documentation updated to reflect current state. |
-| 2026-04-22 | Phase 4 upstream sweep: full audit of all tcsh-org/tcsh open + recently closed issues/PRs. Applied: `sh.file.c` 32-bit `wcscoll` cast (#116); `config_f.h` Shift-JIS `<= 4` condition (#115); `sh.h` `AUTOSET_KANJI` CODESET guard removal (#115). Confirmed already-present: #103, #104, #99, #101, #110. Rejected (upstream closed/not merged): #118 FIONREAD, #114 Shift-JIS runtime check. ISSUES.md, PLAN.md, README.md all updated. |
+| 2026-04-21 | Phase 4/6 review response (dev4): Three flagged weaknesses addressed. Short-circuit fix: `sh.dol.c` Dgetdol() returns STRNULL for unset vars instead of ERR_UNDVAR. Unicode regression documented (no upstream fix available). Test suite created: `tests/` with 7 scripts + Makefile. Gemini inline fixes: `cache_store()` goto removed (two-loop LRU); `GIT_POLL_INTERVAL` named constant. README Known Limitations section added. ISSUES.md Round 6 appended. |
+| 2026-04-21 | Phase 4 upstream sweep: full audit of all tcsh-org/tcsh open + recently closed issues/PRs. Applied: `sh.file.c` 32-bit `wcscoll` cast (#116); `config_f.h` Shift-JIS `<= 4` condition (#115); `sh.h` `AUTOSET_KANJI` CODESET guard removal (#115). Confirmed already-present: #103, #104, #99, #101, #110. Rejected (upstream closed/not merged): #118 FIONREAD, #114 Shift-JIS runtime check. ISSUES.md, PLAN.md, README.md all updated. |
+| 2026-04-21 | PR #5 Round 7 (Copilot + Gemini full review): `sh.dol.c` unset-var modifier fix (call `fixDolMod()` before eatbrac; return 0 for `$#unset`/`$%unset`). `tests/run_tests.sh` portability (glob guard, printf, comment fix). `t006` mktemp portability. `ed.syntax.c` ST_VARIABLE state machine fix ($$/$!/`$<` single-char specials; `>!` / `>>!` / `>\|` operator colouring; remove unused `in_table` var). `ed.chared.c` command+file-aware ghost-text predictor (`predict_file`, `predict_cmd`). New test `t008_unset_modifiers.sh`. ISSUES.md Round 7 appended. |
+| 2026-04-27 | PR #5 Round 9 review response: `ed.syntax.c` redirection coloring fix; `ed.chared.c` caching, `set predict` toggle, `~user` expansion, and empty PATH component handling; test suite hardening (SKIP support, portability, robustness). ISSUES.md Round 9 appended. |

README.md

@@ -47,7 +47,7 @@ mcsh is a drop-in replacement for tcsh and csh:
 
 | Feature | `set` variable | Description |
 |---------|----------------|-------------|
-| **Fish-style predictive autocomplete** | *(always active)* | As you type, the most recent matching history entry is shown as inline ghost text (dimmed). Press Right-Arrow or `^F` to accept the full suggestion. |
+| **Fish-style predictive autocomplete** | `set predict` | As you type, the most recent matching history entry, file path, or command is shown as inline ghost text (dimmed). Press Right-Arrow or `^F` to accept the full suggestion. Includes a filesystem/PATH cache to ensure zero latency. |
 | **Interactive syntax highlighting** | `set syntax` | Per-keystroke ANSI colour highlighting of keywords, builtins, commands (ok/bad), operators, variables, strings (double/single/backtick), comments, and unmatched-quote errors. A 32-entry LRU cache avoids repeated `stat(2)` calls per `$PATH` lookup. |
 | **Filetype colouring in completion** | `set color` | Coloured filetype indicators in tab-completion listings, driven by `LSCOLORS` / `LS_COLORS`. |
 
@@ -286,6 +286,51 @@ Sections and what they provide:
 
 ---
 
+## Resolved regressions
+
+### Unicode / wide-character handling (Round 9)
+
+Multi-byte characters — emoji, CJK, Latin Extended, and any character whose
+UTF-8 encoding is longer than one byte — were previously silently dropped or
+corrupted during filename glob expansion and variable assignment. This was a
+byte-vs-character length bug inherited from tcsh 6.24.14 (tracked as tcsh
+issues #117 / #121).
+
+**Resolution (`sh.lex.c`, `sh.dol.c`):** the `mbtowc` accumulation loops in
+`wide_read()` and the `$<` line-read primitive now bound partial-byte
+lookahead by the runtime `MB_CUR_MAX` instead of the compile-time
+`MB_LEN_MAX`. After a stray invalid byte the loop no longer over-reads up to
+15 bytes of subsequent valid UTF-8.
+
+**Tests:** `tests/t009_unicode_vars.sh` through
+`tests/t014_unicode_script_source.sh` cover variable round-trip,
+`$%` character count, glob expansion, `$<` stdin read, backquote
+substitution, invalid-byte recovery, and sourced-script Unicode.
+See `ISSUES.md` Round 9 for details.
+
+### Short-circuit evaluation (dev4, improved in Round 7)
+
+`if ($?a && "$a" != "")` previously threw "Undefined variable" even when
+`$a` was unset because `Dfix()` expanded all `$` tokens before `&&`
+short-circuiting could suppress evaluation. Fixed in `sh.dol.c`: unset
+variables now silently expand to `""`, matching bash/zsh semantics.
+Modifier expressions like `${unset:h}` and dimen expressions like `$#unset`
+also no longer error — they return `""` and `0` respectively.
+See `ISSUES.md` Rounds 6–7.
+
+### Test coverage
+
+An initial regression suite is in `tests/` covering startup variables,
+arithmetic overflow/shift semantics, short-circuit evaluation, pipe-to-var,
+directory stack, the `function` builtin, signed right-shift, and unset variable
+modifier handling. This covers the core new features but is not exhaustive. Run with:
+
+```sh
+make -C tests MCSH=./mcsh check
+```
+
+---
+
 ## Licensing
 
 mcsh is BSD 3-Clause (see `LICENSE`). The upstream tcsh / etcsh source is also BSD 3-Clause (see `UPSTREAM-COPYRIGHT`). Redistribution must carry both notices — see `NOTICE` for details.