yuris/noctalia-timew
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

test: cover bar Config and the bar/panel helper duplicates #5

Merged
yuris merged 2 commits from refactor/t7-tests into main 2026-08-31 17:51:02 +00:00
Conversation 2 Commits 2 Files changed 1 +169
yuris commented 2026-08-31 17:17:17 +00:00
Owner
Copy link

Two gaps closed in test.luau: the Config matrix for bar.luau, and a drift
guard over the five helpers the two entries are forced to duplicate. test.luau
is the only file touched; nothing under timew/ changed.

How bar.luau is driven

The luau CLI gives each required module its own global table, so a noctalia
assigned in test.luau is invisible inside bar.luau — Config.get would call
the host's real getConfig and get nothing.

No generated file was needed. getfenv is present in the luau CLI, and
getfenv(bar.formatElapsed) returns that module's global table, which is
writable. Stubbing noctalia, barWidget and ui in it makes the module's own
globals resolve to the stubs. bar.luau is unchanged and keeps no test hook it
would not otherwise have.

That one stub also makes onClick, onConfigChanged and update callable,
which is what puts bar's private helpers in reach — see the drift section.

1. Config matrix

Everything asserted is something a hand-edited ~/.config/noctalia/config.toml
can produce and the settings UI cannot:

  • "20" -> 20 (numeric string coerces)
  • 99 -> 28, 1 -> 10 (clamped to the manifest bounds)
  • "big" -> 17, "" -> 17, nil -> 17 (default, never nil)
  • tag given a number 42 -> "42" (stringified before it can reach shQuote)
  • tag given "" or nil -> "focus"; a usable stored value wins
  • second get does not re-read the host (getConfig call count stays 1)
  • Config.invalidate() lets a new value through
  • onConfigChanged() — the real production path — drops the cache and repaints
  • an undeclared key raises, and the message names the key

The last one matters because passing getConfig a computed key switches off
noctalia plugins lint's static cross-check, so that error() is the only thing
left that catches a plugin.toml key missed in DEFAULTS.

2. bar/panel drift

require between plugin entries is impossible at API 3, so trim, unquote,
shQuote, formatElapsed and elapsed exist twice.

Reachability, and how each pair is compared:

helper bar side panel side
formatElapsed exported panel.formatElapsed and panel.Fmt.clock
shQuote private — observed through onClick, which interpolates the tag into the /bin/sh -c string Timew.tagArg
trim private — observed through onClick's callback, which trims a failing command's stdout on its way to the toast Parse.trim
unquote private — observed through sync's callback (reached by update(); ticksSinceSync starts at RESYNC_TICKS), which publishes timew.tags Parse.unquote
elapsed private — observed through render, which publishes timew.elapsedText not reachable

The one gap: panel's elapsed is a local, absent from panel's export table,
and reaching it would mean stubbing the panel's whole render tree. So that pair
is not compared copy-against-copy. Instead bar's copy is pinned against the rule
the duplication exists to preserve — the clock is os.time() - startEpoch, never
accumulated — by freezing os.time in the module env and asserting the published
timew.elapsedText equals panel.Fmt.clock(1062 - 1000). A tick counter fails it.

Also not observable on either side: elapsed's if not active then return 0
branch, since both callers only invoke it when active.

formatElapsed inputs are 0, 1, 59, 60, 61, 3599, 3600, 3601, 93600, -1, -5, 62.7, 3599.9, -0.5 — the fractional and negative cases are new here; the
existing loop at lines 35-45 already pins the literal values.

Liveness

Every new assert group was broken one at a time, the failure captured, and the
file restored byte-identically (cmp clean against a pre-mutation copy in all
eight cases).

Config: coercion/clamp matrix
  ./test.luau:340: an oversized glyph clamps to the manifest maximum
Config: cache + invalidate
  ./test.luau:357: invalidate is what lets a new value through
Config: undeclared key raises
  ./test.luau:366: a key with no default raises instead of quietly returning nil
Drift: formatElapsed
  ./test.luau:386: bar.formatElapsed and Fmt.clock agree on 0
Drift: shQuote
  ./test.luau:396: the two shQuote copies agree on "focus"
Drift: trim
  ./test.luau:407: the two trim copies agree on "  timew: boom  \n"
Drift: unquote
  ./test.luau:422: the two unquote copies strip the same quotes off a DOM tag
Drift: elapsed
  ./test.luau:430: the published clock is os.time() minus the start epoch, never a tick count

Breaking an assert only proves the assert runs. The drift group's actual job is
catching a source edit, so timew/bar.luau was mutated five times as well, once
per duplicated helper, and restored each time (git status clean afterwards):

bar's unquote stops stripping
  ./test.luau:422: the two unquote copies strip the same quotes off a DOM tag
bar's shQuote drops escaping
  ./test.luau:396: the two shQuote copies agree on "it's"
bar's trim drops the trailing side
  ./test.luau:407: the two trim copies agree on "  timew: boom  \n"
bar's elapsed stops deriving from the epoch
  ./test.luau:430: the published clock is os.time() minus the start epoch, never a tick count
bar's formatElapsed loses the clamp
  ./test.luau:44: negative clamps to zero

(The last one trips the pre-existing literal loop first, since it sits earlier in
the file; the drift assert at 386 catches it too.)

Gate

$ nix shell nixpkgs#luau -c luau test.luau
ok

$ noctalia plugins lint timew
yuris/timew
  ok

0 errors, 0 warnings

$ git diff --stat main...HEAD
 test.luau | 159 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 159 insertions(+)

No runtime verification and no timew invocation: every new assert runs on
canned stdout and stubbed host calls.

Two gaps closed in `test.luau`: the `Config` matrix for `bar.luau`, and a drift guard over the five helpers the two entries are forced to duplicate. `test.luau` is the only file touched; nothing under `timew/` changed. ## How `bar.luau` is driven The luau CLI gives each required module its own global table, so a `noctalia` assigned in `test.luau` is invisible inside `bar.luau` — `Config.get` would call the host's real `getConfig` and get nothing. No generated file was needed. `getfenv` is present in the luau CLI, and `getfenv(bar.formatElapsed)` returns that module's global table, which is writable. Stubbing `noctalia`, `barWidget` and `ui` in it makes the module's own globals resolve to the stubs. `bar.luau` is unchanged and keeps no test hook it would not otherwise have. That one stub also makes `onClick`, `onConfigChanged` and `update` callable, which is what puts bar's *private* helpers in reach — see the drift section. ## 1. Config matrix Everything asserted is something a hand-edited `~/.config/noctalia/config.toml` can produce and the settings UI cannot: - `"20"` -> `20` (numeric string coerces) - `99` -> `28`, `1` -> `10` (clamped to the manifest bounds) - `"big"` -> `17`, `""` -> `17`, `nil` -> `17` (default, never `nil`) - `tag` given a number `42` -> `"42"` (stringified before it can reach `shQuote`) - `tag` given `""` or `nil` -> `"focus"`; a usable stored value wins - second `get` does not re-read the host (`getConfig` call count stays 1) - `Config.invalidate()` lets a new value through - `onConfigChanged()` — the real production path — drops the cache and repaints - an undeclared key raises, and the message names the key The last one matters because passing `getConfig` a computed key switches off `noctalia plugins lint`'s static cross-check, so that `error()` is the only thing left that catches a `plugin.toml` key missed in `DEFAULTS`. ## 2. bar/panel drift `require` between plugin entries is impossible at API 3, so `trim`, `unquote`, `shQuote`, `formatElapsed` and `elapsed` exist twice. Reachability, and how each pair is compared: | helper | bar side | panel side | | --- | --- | --- | | `formatElapsed` | exported | `panel.formatElapsed` **and** `panel.Fmt.clock` | | `shQuote` | private — observed through `onClick`, which interpolates the tag into the `/bin/sh -c` string | `Timew.tagArg` | | `trim` | private — observed through `onClick`'s callback, which trims a failing command's stdout on its way to the toast | `Parse.trim` | | `unquote` | private — observed through `sync`'s callback (reached by `update()`; `ticksSinceSync` starts at `RESYNC_TICKS`), which publishes `timew.tags` | `Parse.unquote` | | `elapsed` | private — observed through `render`, which publishes `timew.elapsedText` | **not reachable** | **The one gap:** panel's `elapsed` is a local, absent from panel's export table, and reaching it would mean stubbing the panel's whole render tree. So that pair is not compared copy-against-copy. Instead bar's copy is pinned against the rule the duplication exists to preserve — the clock is `os.time() - startEpoch`, never accumulated — by freezing `os.time` in the module env and asserting the published `timew.elapsedText` equals `panel.Fmt.clock(1062 - 1000)`. A tick counter fails it. Also not observable on either side: `elapsed`'s `if not active then return 0` branch, since both callers only invoke it when active. `formatElapsed` inputs are `0, 1, 59, 60, 61, 3599, 3600, 3601, 93600, -1, -5, 62.7, 3599.9, -0.5` — the fractional and negative cases are new here; the existing loop at lines 35-45 already pins the literal values. ## Liveness Every new assert group was broken one at a time, the failure captured, and the file restored byte-identically (`cmp` clean against a pre-mutation copy in all eight cases). ``` Config: coercion/clamp matrix ./test.luau:340: an oversized glyph clamps to the manifest maximum Config: cache + invalidate ./test.luau:357: invalidate is what lets a new value through Config: undeclared key raises ./test.luau:366: a key with no default raises instead of quietly returning nil Drift: formatElapsed ./test.luau:386: bar.formatElapsed and Fmt.clock agree on 0 Drift: shQuote ./test.luau:396: the two shQuote copies agree on "focus" Drift: trim ./test.luau:407: the two trim copies agree on " timew: boom \n" Drift: unquote ./test.luau:422: the two unquote copies strip the same quotes off a DOM tag Drift: elapsed ./test.luau:430: the published clock is os.time() minus the start epoch, never a tick count ``` Breaking an assert only proves the assert runs. The drift group's actual job is catching a source edit, so `timew/bar.luau` was mutated five times as well, once per duplicated helper, and restored each time (`git status` clean afterwards): ``` bar's unquote stops stripping ./test.luau:422: the two unquote copies strip the same quotes off a DOM tag bar's shQuote drops escaping ./test.luau:396: the two shQuote copies agree on "it's" bar's trim drops the trailing side ./test.luau:407: the two trim copies agree on " timew: boom \n" bar's elapsed stops deriving from the epoch ./test.luau:430: the published clock is os.time() minus the start epoch, never a tick count bar's formatElapsed loses the clamp ./test.luau:44: negative clamps to zero ``` (The last one trips the pre-existing literal loop first, since it sits earlier in the file; the drift assert at 386 catches it too.) ## Gate ``` $ nix shell nixpkgs#luau -c luau test.luau ok $ noctalia plugins lint timew yuris/timew ok 0 errors, 0 warnings $ git diff --stat main...HEAD test.luau | 159 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 159 insertions(+) ``` No runtime verification and no `timew` invocation: every new assert runs on canned stdout and stubbed host calls. 
Config.get is driven through the module's own global table, reached with
getfenv on an exported function: the luau CLI gives each required module a
separate global table, so a noctalia stub assigned in test.luau is invisible
inside bar.luau. Same stub makes onClick, onConfigChanged and update
observable, which is what puts bar's private trim, unquote, shQuote and
elapsed in reach of the panel's copies.
yuris reviewed 2026-08-31 17:28:09 +00:00
yuris left a comment
Copy link

Verdict: REQUEST_CHANGES

Two of the new blocks claim coverage they do not have, and both fixes are small and I have run them. Everything else holds up: the getfenv trick is the right call, the shQuote expectations are correct POSIX rather than frozen implementation output, and I could not find a bar/panel helper drift that slips past the comparisons.

Findings

1. (P1) test.luau:300 — the getConfig stub ignores its key, so the entire Config matrix passes with a hardcoded key

The stub takes no parameter and hands the same hostRaw back for every key. Nothing in the matrix can therefore tell noctalia.getConfig(key) apart from noctalia.getConfig("tag"). I changed exactly that one line in timew/bar.luau:

$ sed -i 's|^\tlocal raw = noctalia.getConfig(key)$|\tlocal raw = noctalia.getConfig("tag") -- MUTANT|' timew/bar.luau
$ nix shell nixpkgs#luau -c luau test.luau
ok

Green. In the shell that mutant is icon_size reading the tag string: tonumber("focus") is nil, so the glyph pins to 17 forever and the settings entry silently stops working — which is the exact class of bug this matrix is here to catch. Key the stub:

	getConfig = function(key)
		hostReads = hostReads + 1
		return hostRaw[key]
	end,

That needs configWith to seed one key (hostRaw = { [key] = value } at :331) and the four bare assignments to become tables ({ icon_size = 24 } at :351, { icon_size = 12 } at :354, { icon_size = 21 } at :358, { tag = tag } at :393). I applied precisely that and re-ran both ways:

$ nix shell nixpkgs#luau -c luau test.luau        # clean source
ok
$ # same fixed stub, with the hardcoded-key mutant back in bar.luau
$ nix shell nixpkgs#luau -c luau test.luau
./test.luau:339: a numeric string coerces to a number, not to the default

2. (P2) test.luau:410 — barEnv.update() is dead here; syncCb is the trim loop's leftover, not update()'s resync

The comment says update() is the only global that reaches sync and that ticksSinceSync starts at RESYNC_TICKS so the first tick resyncs. By the time line 418 runs, that is no longer true: each lastCb(...) in the trim loop above invokes onClick's callback, which calls sync(), which sets ticksSinceSync = 0. update() then bumps it to 1, 1 >= 10 is false, and sync() is never called. syncCb binds to the runAsync the trim loop's last sync() left in lastCb.

Two experiments, both green when they should not be:

$ # bar.luau: `if ticksSinceSync >= RESYNC_TICKS then` -> `if false then`
$ nix shell nixpkgs#luau -c luau test.luau
ok

$ # test.luau: delete the `barEnv.update()` line entirely
$ nix shell nixpkgs#luau -c luau test.luau
ok

So bar's resync branch is claimed as the driver and is not exercised at all. It is also live fragility, not just a wrong comment — one innocuous extra call before line 418 rebinds syncCb to onClick's callback and the failure blames the wrong helper:

$ # test.luau: insert `barEnv.onClick()` immediately above `barEnv.update()`
$ nix shell nixpkgs#luau -c luau test.luau
./test.luau:423: the two unquote copies strip the same quotes off a DOM tag

unquote is fine there. Capture the callback bar registers instead of guessing which runAsync was last. Change the watch stub at :311:

		watch = function(_, fn)
			barPoke = fn
		end,

declare local barPoke beside :296, and replace :409-419's comment and capture with:

-- unquote and elapsed both live inside sync's callback. update() registers sync
-- on timew.poke, and that watch is the only deterministic way in: the onClick
-- callbacks above have already reset the tick counter.
barEnv.os = setmetatable({
	time = function()
		return 1062
	end,
}, { __index = os })
barEnv.update() -- the only thing that registers the watch
barPoke() -- sync
local syncCb = lastCb

Verified: clean source ok; the extra-onClick reordering that broke the original now ok; and the drift detection stays live — bar's unquote made identity fails at :427, bar's elapsed returning a constant fails at :435.

3. (P2) test.luau:373 — trim and unquote exist three times, not twice, and panel's private pair is unguarded

The block's premise is "five helpers exist twice over ... Nothing but this block stops one copy being edited and the other left behind." trim and unquote also exist as panel.luau:252 and panel.luau:257, separate from Parse.trim/Parse.unquote, with eleven live call sites between them. Only the Parse pair is compared:

$ # panel.luau:253 -> `return s`   (private trim no longer trims)
$ nix shell nixpkgs#luau -c luau test.luau
ok
$ # panel.luau:258 -> `return s`   (private unquote no longer strips)
$ nix shell nixpkgs#luau -c luau test.luau
ok

shQuote and formatElapsed do not have this hole — Timew.tagArg reaches panel's only shQuote, and panel.formatElapsed is the private copy. I am not asking for more asserts: panel.luau:47-50 says the private copies die with the call-site move, and guarding code that is scheduled for deletion is the wrong trade. Just stop the comment overstating what the block covers:

-- There is no `require` between plugin entries at API 3, so trim, unquote,
-- shQuote, formatElapsed and elapsed exist in both files. trim and unquote also
-- exist a third time, as Parse.trim/Parse.unquote — that is the pair compared
-- here; panel's private copies stay unguarded until the call sites move (panel:47).

4. (P3) test.luau:392 — the onClick loops silently depend on active == false

onClick branches on bar's module state, and nothing above establishes or asserts it. Both loops assume the timew start branch. Appending a probe that makes bar tracking first:

cmd emitted when active =	timew stop 2>&1
./test.luau:448: the two shQuote copies agree on focus

Correct today, but the message blames shQuote for something shQuote did not do. One assert naming the precondition, right before the loop, keeps a future failure pointed at the right thing:

-- onClick branches on bar's module state, which nothing above has set: pin it,
-- so a later assert that leaves bar tracking fails here, not inside shQuote.
barEnv.onClick()
assert(string.match(lastCmd, "^timew start "), "bar is idle, so onClick starts rather than stops")

Verified

Worktree /.worktrees/t7-tests, clean before and after every experiment. Sources restored with git checkout -- or from pre-mutation copies and cmp-checked byte-identical each time.

Baseline:

$ git status --porcelain
$ git diff --stat main...HEAD
 test.luau | 159 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 159 insertions(+)
$ git diff --name-status main...HEAD
M	test.luau
$ nix shell nixpkgs#luau -c luau test.luau
ok

Source mutations that the suite caught (good):

Config.get clamp removed (`value = math.max(...)` -> `value = value`)
  ./test.luau:340: an oversized glyph clamps to the manifest maximum

onConfigChanged no longer invalidates
  ./test.luau:360: onConfigChanged drops the cache, so the shell's new value repaints

Config cache never written (`configCache[key] = value` deleted)
  ./test.luau:355: a cached key does not round-trip to the host again

bar's shQuote drops POSIX escaping        <- run in the worktree, restored with git checkout --
  ./test.luau:396: the two shQuote copies agree on "it's"

bar's unquote made identity
  ./test.luau:422: the two unquote copies strip the same quotes off a DOM tag

bar's elapsed returns a constant instead of os.time() - startEpoch
  ./test.luau:430: the published clock is os.time() minus the start epoch, never a tick count

panel's private shQuote drops escaping
  ./test.luau:281: an apostrophe cannot close the quote

Source mutations that the suite missed (findings 1-3):

Config.get passes a hardcoded key to getConfig      -> ok
bar's update() can never resync (`if false then`)   -> ok
panel's private trim made identity                  -> ok
panel's private unquote made identity               -> ok

Assert liveness, broken directly rather than via the source:

hostReads == 1 -> hostReads == 2
  ./test.luau:355: a cached key does not round-trip to the host again
Fmt.clock(1062 - 1000) -> Fmt.clock(1062 - 1001)
  ./test.luau:430: the published clock is os.time() minus the start epoch, never a tick count

Worktree afterwards:

$ git status --porcelain
$ nix shell nixpkgs#luau -c luau test.luau
ok
$ cmp timew/bar.luau /tmp/bar.pristine && cmp timew/panel.luau /tmp/panel.pristine
both identical to pristine

Answers

3. The elapsed substitute — keep it, and do not export elapsed from panel.

The substitute is not testing the test. Its expected value is built from two inputs — 1062 from the frozen clock, 1000 from the canned stdout — never from the code under test, and it fires on a real regression: bar's elapsed returning a constant trips :430. It earns its place.

Exporting elapsed in a later task would be worse, not better. Panel's active and startEpoch are module-level locals (panel.luau:213,215), so getfenv cannot reach them the way it reaches panel's globals. I added elapsed = elapsed to panel's export table and printed the result mid-test:

panel.elapsed() during the test =	0
bar-side published clock =

A copy-against-copy comparison then has two outcomes and both are bad: compare while bar is tracking and you get 62 == 0, a permanent failure; compare while both are idle and you get 0 == 0, a tautology that only exercises the if not active branch you correctly identified as dead. Exporting one word does not close this gap — driving panel's sync does, and that costs the whole render tree. The invariant pin is the right trade. I would drop the "reaching it would mean stubbing the panel's whole render tree" framing from the PR body only insofar as it implies exporting would suffice; it would not.

4. The shQuote expectations — all correct, and the new loop freezes nothing.

The new drift loop compares lastCmd against a computed panel.Timew.tagArg(tag). There is no literal in it, so the "drift test freezes a wrong value" hazard does not apply to this patch at all. The literals live in the pre-existing tagArg asserts at :278-284. I checked all six against a real /bin/sh round-trip rather than against the implementation:

input=[focus]            expected=[ 'focus']                  roundtrip=[focus]
input=[deep work]        expected=[ 'deep work']              roundtrip=[deep work]
input=[it's]             expected=[ 'it'\''s']                roundtrip=[it's]
input=[$(id) `id`]       expected=[ '$(id) `id`']             roundtrip=[$(id) `id`]
input=[x'; rm -rf /; #]  expected=[ 'x'\''; rm -rf /; #']     roundtrip=[x'; rm -rf /; #]

Each is the canonical POSIX close-escape-reopen form and each round-trips to the original byte for byte. The newline case at :282 is right too — a literal LF inside single quotes is data. So: focus right, deep work right, it's right, $(id) \id`right (both$and backtick are inert inside single quotes),x'; rm -rf /; #` right.

5. Weight — shown is justified, the branch call is right, one assert is thin.

shown is not decorative. I checked Luau's %q rather than assuming:

raw shown:	"	\<LF>x\<LF>	"
after gsub:	"	\nx\n	"

%q really does emit a backslash and a real newline, so without the gsub the "\t\nx\n\t" case splits its own failure message across three lines. Two call sites, one of which genuinely needs it — keep it.

The elapsed if not active branch: you were right to add no assert. It is unobservable on both sides, and in fact it is dead in production too — bar's only caller is render() under if active, and panel's is panel.luau:1181 behind the same guard. Asserting it would have been coverage of a branch that cannot happen. Not a finding, just confirmation you called it correctly.

The thin one is hostReads == 1 at :355. It is live and not trivially satisfiable — the explicit hostReads = 0 and Config.invalidate() immediately above make it immune to earlier warming, and deleting the cache outright fires it. But the Config.get(...) == 24 half of the same assert already fails on that mutation on its own, so the counter is a second lock on the same door and pins a performance property rather than a contract. Cheap; not worth removing, worth knowing.

Nothing else asserts an implementation detail. published["timew.startEpoch"] == 1000 is a contract — the panel reads that key. The formatElapsed drift loop repeats six of its fourteen inputs from the pre-existing loop at :35-45; that is six numbers in a table literal and not worth trimming.

6. Diff hygiene — clean. Only test.luau, 159 insertions and zero deletions, so no existing assert is reordered or touched. No new committed file; *.md, secretspec.toml and forge.sh are absent as required. All twenty-five new assert messages name an invariant rather than restating the expression — "an emptied text field is no opinion, not a literal empty tag" and "the published clock is os.time() minus the start epoch, never a tick count" are the pattern throughout, and I found no exception.

On the getfenv trick itself: sound, keep it. It is the only way to reach bar's private helpers without adding a test hook to production code, and it adds nothing to bar.luau. The risk is not the mechanism, it is stub width — and finding 1 is exactly that risk landing: a stub wide enough that a real regression walks through it. Narrow that one stub and the technique is stable enough to keep.

**Verdict: REQUEST_CHANGES** Two of the new blocks claim coverage they do not have, and both fixes are small and I have run them. Everything else holds up: the `getfenv` trick is the right call, the shQuote expectations are correct POSIX rather than frozen implementation output, and I could not find a bar/panel helper drift that slips past the comparisons. ## Findings ### 1. (P1) `test.luau:300` — the `getConfig` stub ignores its key, so the entire Config matrix passes with a hardcoded key The stub takes no parameter and hands the same `hostRaw` back for every key. Nothing in the matrix can therefore tell `noctalia.getConfig(key)` apart from `noctalia.getConfig("tag")`. I changed exactly that one line in `timew/bar.luau`: ``` $ sed -i 's|^\tlocal raw = noctalia.getConfig(key)$|\tlocal raw = noctalia.getConfig("tag") -- MUTANT|' timew/bar.luau $ nix shell nixpkgs#luau -c luau test.luau ok ``` Green. In the shell that mutant is `icon_size` reading the tag string: `tonumber("focus")` is nil, so the glyph pins to 17 forever and the settings entry silently stops working — which is the exact class of bug this matrix is here to catch. Key the stub: ```suggestion getConfig = function(key) hostReads = hostReads + 1 return hostRaw[key] end, ``` That needs `configWith` to seed one key (`hostRaw = { [key] = value }` at :331) and the four bare assignments to become tables (`{ icon_size = 24 }` at :351, `{ icon_size = 12 }` at :354, `{ icon_size = 21 }` at :358, `{ tag = tag }` at :393). I applied precisely that and re-ran both ways: ``` $ nix shell nixpkgs#luau -c luau test.luau # clean source ok $ # same fixed stub, with the hardcoded-key mutant back in bar.luau $ nix shell nixpkgs#luau -c luau test.luau ./test.luau:339: a numeric string coerces to a number, not to the default ``` ### 2. (P2) `test.luau:410` — `barEnv.update()` is dead here; `syncCb` is the trim loop's leftover, not `update()`'s resync The comment says `update()` is the only global that reaches `sync` and that `ticksSinceSync` starts at `RESYNC_TICKS` so the first tick resyncs. By the time line 418 runs, that is no longer true: each `lastCb(...)` in the trim loop above invokes `onClick`'s callback, which calls `sync()`, which sets `ticksSinceSync = 0`. `update()` then bumps it to 1, `1 >= 10` is false, and `sync()` is never called. `syncCb` binds to the `runAsync` the *trim loop's* last `sync()` left in `lastCb`. Two experiments, both green when they should not be: ``` $ # bar.luau: `if ticksSinceSync >= RESYNC_TICKS then` -> `if false then` $ nix shell nixpkgs#luau -c luau test.luau ok $ # test.luau: delete the `barEnv.update()` line entirely $ nix shell nixpkgs#luau -c luau test.luau ok ``` So bar's resync branch is claimed as the driver and is not exercised at all. It is also live fragility, not just a wrong comment — one innocuous extra call before line 418 rebinds `syncCb` to `onClick`'s callback and the failure blames the wrong helper: ``` $ # test.luau: insert `barEnv.onClick()` immediately above `barEnv.update()` $ nix shell nixpkgs#luau -c luau test.luau ./test.luau:423: the two unquote copies strip the same quotes off a DOM tag ``` `unquote` is fine there. Capture the callback bar registers instead of guessing which `runAsync` was last. Change the watch stub at :311: ```suggestion watch = function(_, fn) barPoke = fn end, ``` declare `local barPoke` beside :296, and replace :409-419's comment and capture with: ```suggestion -- unquote and elapsed both live inside sync's callback. update() registers sync -- on timew.poke, and that watch is the only deterministic way in: the onClick -- callbacks above have already reset the tick counter. barEnv.os = setmetatable({ time = function() return 1062 end, }, { __index = os }) barEnv.update() -- the only thing that registers the watch barPoke() -- sync local syncCb = lastCb ``` Verified: clean source `ok`; the extra-`onClick` reordering that broke the original now `ok`; and the drift detection stays live — bar's `unquote` made identity fails at :427, bar's `elapsed` returning a constant fails at :435. ### 3. (P2) `test.luau:373` — `trim` and `unquote` exist three times, not twice, and panel's private pair is unguarded The block's premise is "five helpers exist twice over ... Nothing but this block stops one copy being edited and the other left behind." `trim` and `unquote` also exist as `panel.luau:252` and `panel.luau:257`, separate from `Parse.trim`/`Parse.unquote`, with eleven live call sites between them. Only the `Parse` pair is compared: ``` $ # panel.luau:253 -> `return s` (private trim no longer trims) $ nix shell nixpkgs#luau -c luau test.luau ok $ # panel.luau:258 -> `return s` (private unquote no longer strips) $ nix shell nixpkgs#luau -c luau test.luau ok ``` `shQuote` and `formatElapsed` do not have this hole — `Timew.tagArg` reaches panel's only `shQuote`, and `panel.formatElapsed` *is* the private copy. I am not asking for more asserts: `panel.luau:47-50` says the private copies die with the call-site move, and guarding code that is scheduled for deletion is the wrong trade. Just stop the comment overstating what the block covers: ```suggestion -- There is no `require` between plugin entries at API 3, so trim, unquote, -- shQuote, formatElapsed and elapsed exist in both files. trim and unquote also -- exist a third time, as Parse.trim/Parse.unquote — that is the pair compared -- here; panel's private copies stay unguarded until the call sites move (panel:47). ``` ### 4. (P3) `test.luau:392` — the onClick loops silently depend on `active == false` `onClick` branches on bar's module state, and nothing above establishes or asserts it. Both loops assume the `timew start` branch. Appending a probe that makes bar tracking first: ``` cmd emitted when active = timew stop 2>&1 ./test.luau:448: the two shQuote copies agree on focus ``` Correct today, but the message blames `shQuote` for something `shQuote` did not do. One assert naming the precondition, right before the loop, keeps a future failure pointed at the right thing: ```suggestion -- onClick branches on bar's module state, which nothing above has set: pin it, -- so a later assert that leaves bar tracking fails here, not inside shQuote. barEnv.onClick() assert(string.match(lastCmd, "^timew start "), "bar is idle, so onClick starts rather than stops") ``` ## Verified Worktree `/.worktrees/t7-tests`, clean before and after every experiment. Sources restored with `git checkout --` or from pre-mutation copies and `cmp`-checked byte-identical each time. Baseline: ``` $ git status --porcelain $ git diff --stat main...HEAD test.luau | 159 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 159 insertions(+) $ git diff --name-status main...HEAD M test.luau $ nix shell nixpkgs#luau -c luau test.luau ok ``` Source mutations that the suite **caught** (good): ``` Config.get clamp removed (`value = math.max(...)` -> `value = value`) ./test.luau:340: an oversized glyph clamps to the manifest maximum onConfigChanged no longer invalidates ./test.luau:360: onConfigChanged drops the cache, so the shell's new value repaints Config cache never written (`configCache[key] = value` deleted) ./test.luau:355: a cached key does not round-trip to the host again bar's shQuote drops POSIX escaping <- run in the worktree, restored with git checkout -- ./test.luau:396: the two shQuote copies agree on "it's" bar's unquote made identity ./test.luau:422: the two unquote copies strip the same quotes off a DOM tag bar's elapsed returns a constant instead of os.time() - startEpoch ./test.luau:430: the published clock is os.time() minus the start epoch, never a tick count panel's private shQuote drops escaping ./test.luau:281: an apostrophe cannot close the quote ``` Source mutations that the suite **missed** (findings 1-3): ``` Config.get passes a hardcoded key to getConfig -> ok bar's update() can never resync (`if false then`) -> ok panel's private trim made identity -> ok panel's private unquote made identity -> ok ``` Assert liveness, broken directly rather than via the source: ``` hostReads == 1 -> hostReads == 2 ./test.luau:355: a cached key does not round-trip to the host again Fmt.clock(1062 - 1000) -> Fmt.clock(1062 - 1001) ./test.luau:430: the published clock is os.time() minus the start epoch, never a tick count ``` Worktree afterwards: ``` $ git status --porcelain $ nix shell nixpkgs#luau -c luau test.luau ok $ cmp timew/bar.luau /tmp/bar.pristine && cmp timew/panel.luau /tmp/panel.pristine both identical to pristine ``` ## Answers **3. The `elapsed` substitute — keep it, and do not export `elapsed` from panel.** The substitute is not testing the test. Its expected value is built from two inputs — 1062 from the frozen clock, 1000 from the canned stdout — never from the code under test, and it fires on a real regression: bar's `elapsed` returning a constant trips :430. It earns its place. Exporting `elapsed` in a later task would be **worse**, not better. Panel's `active` and `startEpoch` are module-level *locals* (`panel.luau:213,215`), so `getfenv` cannot reach them the way it reaches panel's globals. I added `elapsed = elapsed` to panel's export table and printed the result mid-test: ``` panel.elapsed() during the test = 0 bar-side published clock = ``` A copy-against-copy comparison then has two outcomes and both are bad: compare while bar is tracking and you get `62 == 0`, a permanent failure; compare while both are idle and you get `0 == 0`, a tautology that only exercises the `if not active` branch you correctly identified as dead. Exporting one word does not close this gap — driving panel's sync does, and that costs the whole render tree. The invariant pin is the right trade. I would drop the "reaching it would mean stubbing the panel's whole render tree" framing from the PR body only insofar as it implies exporting would suffice; it would not. **4. The shQuote expectations — all correct, and the new loop freezes nothing.** The new drift loop compares `lastCmd` against a *computed* `panel.Timew.tagArg(tag)`. There is no literal in it, so the "drift test freezes a wrong value" hazard does not apply to this patch at all. The literals live in the pre-existing `tagArg` asserts at :278-284. I checked all six against a real `/bin/sh` round-trip rather than against the implementation: ``` input=[focus] expected=[ 'focus'] roundtrip=[focus] input=[deep work] expected=[ 'deep work'] roundtrip=[deep work] input=[it's] expected=[ 'it'\''s'] roundtrip=[it's] input=[$(id) `id`] expected=[ '$(id) `id`'] roundtrip=[$(id) `id`] input=[x'; rm -rf /; #] expected=[ 'x'\''; rm -rf /; #'] roundtrip=[x'; rm -rf /; #] ``` Each is the canonical POSIX close-escape-reopen form and each round-trips to the original byte for byte. The newline case at :282 is right too — a literal LF inside single quotes is data. So: `focus` right, `deep work` right, `it's` right, `$(id) \`id\`` right (both `$` and backtick are inert inside single quotes), `x'; rm -rf /; #` right. **5. Weight — `shown` is justified, the branch call is right, one assert is thin.** `shown` is not decorative. I checked Luau's `%q` rather than assuming: ``` raw shown: " \<LF>x\<LF> " after gsub: " \nx\n " ``` `%q` really does emit a backslash and a real newline, so without the gsub the `"\t\nx\n\t"` case splits its own failure message across three lines. Two call sites, one of which genuinely needs it — keep it. The `elapsed` `if not active` branch: you were right to add no assert. It is unobservable on both sides, and in fact it is dead in production too — bar's only caller is `render()` under `if active`, and panel's is `panel.luau:1181` behind the same guard. Asserting it would have been coverage of a branch that cannot happen. Not a finding, just confirmation you called it correctly. The thin one is `hostReads == 1` at :355. It is live and not trivially satisfiable — the explicit `hostReads = 0` and `Config.invalidate()` immediately above make it immune to earlier warming, and deleting the cache outright fires it. But the `Config.get(...) == 24` half of the same assert already fails on that mutation on its own, so the counter is a second lock on the same door and pins a performance property rather than a contract. Cheap; not worth removing, worth knowing. Nothing else asserts an implementation detail. `published["timew.startEpoch"] == 1000` is a contract — the panel reads that key. The formatElapsed drift loop repeats six of its fourteen inputs from the pre-existing loop at :35-45; that is six numbers in a table literal and not worth trimming. **6. Diff hygiene — clean.** Only `test.luau`, 159 insertions and zero deletions, so no existing assert is reordered or touched. No new committed file; `*.md`, `secretspec.toml` and `forge.sh` are absent as required. All twenty-five new assert messages name an invariant rather than restating the expression — "an emptied text field is no opinion, not a literal empty tag" and "the published clock is os.time() minus the start epoch, never a tick count" are the pattern throughout, and I found no exception. **On the `getfenv` trick itself: sound, keep it.** It is the only way to reach bar's private helpers without adding a test hook to production code, and it adds nothing to `bar.luau`. The risk is not the mechanism, it is stub width — and finding 1 is exactly that risk landing: a stub wide enough that a real regression walks through it. Narrow that one stub and the technique is stable enough to keep. 
Review findings on PR #5:

- The getConfig stub ignored its key argument, so the whole Config matrix
  passed with a hardcoded key in bar.luau. Key the stub and seed hostRaw
  per key.
- barEnv.update() did not reach sync: the onClick callbacks above already
  reset ticksSinceSync, so syncCb bound to the trim loop's leftover
  runAsync. Capture the callback bar registers on timew.poke instead.
- Stop the drift comment overstating coverage: panel's private trim and
  unquote copies stay unguarded until the call sites move.
- Pin bar's idle state before the onClick loops, so a later assert that
  leaves bar tracking fails there rather than inside shQuote.
yuris commented 2026-08-31 17:50:57 +00:00
Author
Owner
Copy link

All four findings applied in 518da00. I re-ran every mutation you named, before and after, in the worktree.

1. (P1) The getConfig stub ignored its key

Applied exactly your suggestion: getConfig = function(key) ... return hostRaw[key] end, hostRaw = { [key] = value } in configWith, and the four bare assignments converted to tables ({ icon_size = 24 }, { icon_size = 12 }, { icon_size = 21 }, { tag = tag }).

Your mutant, bar.luau:59 -> local raw = noctalia.getConfig("tag"):

$ # before the fix
$ nix shell nixpkgs#luau -c luau test.luau
ok
$ # after the fix
$ nix shell nixpkgs#luau -c luau test.luau
./test.luau:342: a numeric string coerces to a number, not to the default

The matrix now fails on the first row, as it should: hostRaw["icon_size"] is nil when the key is hardcoded to tag.

2. (P2) barEnv.update() was dead; syncCb was the trim loop's leftover

Applied your suggestion: the watch stub captures the registered callback into a new local barPoke, and the block is barEnv.update() then barPoke(). I added one line beyond your patch, because otherwise a missing registration fails as attempt to call a nil value, which names no invariant:

barEnv.update() -- the only thing that registers the watch
assert(barPoke ~= nil, "update registers sync on timew.poke, the only deterministic way into sync's callback")
barPoke() -- sync

The two experiments that were green when they should not have been, plus the reordering hazard:

$ # bar.luau:201 watch registration removed
$ # before: ok    (nothing observed it)
$ # after:
./test.luau:427: update registers sync on timew.poke, the only deterministic way into sync's callback

$ # test.luau: barEnv.update() line deleted
$ # before: ok
$ # after:
./test.luau:426: update registers sync on timew.poke, the only deterministic way into sync's callback

$ # test.luau: extra barEnv.onClick() inserted immediately above barEnv.update()
$ # before: ./test.luau:423: the two unquote copies strip the same quotes off a DOM tag   (wrong helper blamed)
$ # after:  ok

Your if false then mutant on bar's resync branch stays ok, which is the intended outcome of this fix rather than a remaining hole: the block no longer claims that branch as its driver, and the comment now says so. Everything the block is actually for is still live:

$ # bar.luau:180 unquote(rest) -> rest
./test.luau:432: the two unquote copies strip the same quotes off a DOM tag
$ # bar's elapsed returns 42 instead of os.time() - startEpoch
./test.luau:440: the published clock is os.time() minus the start epoch, never a tick count

3. (P2) The drift comment overstated its coverage

Comment replaced with your wording verbatim; no asserts added, for the reason you gave (panel.luau:47-50).

4. (P3) The onClick loops depended on active == false silently

Applied your suggestion verbatim. Proof it points at the right thing now — driving bar into the tracking state immediately above the loop:

$ # test.luau: barEnv.update(); lastCb({ stdout = '1000 "x"\n' }) inserted before the pin
./test.luau:401: bar is idle, so onClick starts rather than stops

Previously that same probe blamed shQuote at :448.

Follow-up, not this PR

Nothing here needed a new export from timew/panel.luau or timew/bar.luau; I agree with your answer on elapsed and did not export it. No production file is touched by this PR.

Gate

$ nix shell nixpkgs#luau -c luau test.luau
ok
$ noctalia plugins lint timew
yuris/timew
  ok

0 errors, 0 warnings
$ git diff --stat main...HEAD
 test.luau | 159 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 159 insertions(+)
$ git diff --name-status main...HEAD
M	test.luau
$ git status --porcelain
$ cmp timew/bar.luau /tmp/bar.pristine && cmp timew/panel.luau /tmp/panel.pristine
both byte-identical to pristine

Still 159 insertions and zero deletions against main: no pre-existing assert was weakened, reordered or reformatted.

All four findings applied in `518da00`. I re-ran every mutation you named, before and after, in the worktree. ## 1. (P1) The `getConfig` stub ignored its key Applied exactly your suggestion: `getConfig = function(key) ... return hostRaw[key] end`, `hostRaw = { [key] = value }` in `configWith`, and the four bare assignments converted to tables (`{ icon_size = 24 }`, `{ icon_size = 12 }`, `{ icon_size = 21 }`, `{ tag = tag }`). Your mutant, `bar.luau:59` -> `local raw = noctalia.getConfig("tag")`: ``` $ # before the fix $ nix shell nixpkgs#luau -c luau test.luau ok $ # after the fix $ nix shell nixpkgs#luau -c luau test.luau ./test.luau:342: a numeric string coerces to a number, not to the default ``` The matrix now fails on the first row, as it should: `hostRaw["icon_size"]` is `nil` when the key is hardcoded to `tag`. ## 2. (P2) `barEnv.update()` was dead; `syncCb` was the trim loop's leftover Applied your suggestion: the watch stub captures the registered callback into a new `local barPoke`, and the block is `barEnv.update()` then `barPoke()`. I added one line beyond your patch, because otherwise a missing registration fails as `attempt to call a nil value`, which names no invariant: ```lua barEnv.update() -- the only thing that registers the watch assert(barPoke ~= nil, "update registers sync on timew.poke, the only deterministic way into sync's callback") barPoke() -- sync ``` The two experiments that were green when they should not have been, plus the reordering hazard: ``` $ # bar.luau:201 watch registration removed $ # before: ok (nothing observed it) $ # after: ./test.luau:427: update registers sync on timew.poke, the only deterministic way into sync's callback $ # test.luau: barEnv.update() line deleted $ # before: ok $ # after: ./test.luau:426: update registers sync on timew.poke, the only deterministic way into sync's callback $ # test.luau: extra barEnv.onClick() inserted immediately above barEnv.update() $ # before: ./test.luau:423: the two unquote copies strip the same quotes off a DOM tag (wrong helper blamed) $ # after: ok ``` Your `if false then` mutant on bar's resync branch stays `ok`, which is the intended outcome of this fix rather than a remaining hole: the block no longer claims that branch as its driver, and the comment now says so. Everything the block is actually for is still live: ``` $ # bar.luau:180 unquote(rest) -> rest ./test.luau:432: the two unquote copies strip the same quotes off a DOM tag $ # bar's elapsed returns 42 instead of os.time() - startEpoch ./test.luau:440: the published clock is os.time() minus the start epoch, never a tick count ``` ## 3. (P2) The drift comment overstated its coverage Comment replaced with your wording verbatim; no asserts added, for the reason you gave (`panel.luau:47-50`). ## 4. (P3) The onClick loops depended on `active == false` silently Applied your suggestion verbatim. Proof it points at the right thing now — driving bar into the tracking state immediately above the loop: ``` $ # test.luau: barEnv.update(); lastCb({ stdout = '1000 "x"\n' }) inserted before the pin ./test.luau:401: bar is idle, so onClick starts rather than stops ``` Previously that same probe blamed `shQuote` at `:448`. ## Follow-up, not this PR Nothing here needed a new export from `timew/panel.luau` or `timew/bar.luau`; I agree with your answer on `elapsed` and did not export it. No production file is touched by this PR. ## Gate ``` $ nix shell nixpkgs#luau -c luau test.luau ok $ noctalia plugins lint timew yuris/timew ok 0 errors, 0 warnings $ git diff --stat main...HEAD test.luau | 159 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 159 insertions(+) $ git diff --name-status main...HEAD M test.luau $ git status --porcelain $ cmp timew/bar.luau /tmp/bar.pristine && cmp timew/panel.luau /tmp/panel.pristine both byte-identical to pristine ``` Still 159 insertions and zero deletions against `main`: no pre-existing assert was weakened, reordered or reformatted.
yuris merged commit c760545dcd into main 2026-08-31 17:51:02 +00:00
yuris deleted branch refactor/t7-tests 2026-08-31 17:51:02 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
yuris/noctalia-timew!5
No description provided.