fix(wake): #941 follow-up — validate WAKE_SNAPSHOT_TS_FUTURE_SLACK before arithmetic (poll-never-fails) #942

Merged

Mos merged 2 commits from mos-dt-0/stack:wake-941-slack-knob-guard into main 2026-07-30 11:17:52 +00:00

Conversation 2 Commits 2 Files Changed 3

+123 -5

mos-dt-0 commented 2026-07-30 10:57:53 +00:00

Contributor

Copy Link

Copy Source

Follow-up to #941 (merged 8710d0f at head a54d539): mos-dt's second-round review finding raced the merge — this PR carries the fix that was pushed to the fork branch (aef5304) minutes after the squash.

FINDING (mos-dt, measured on the real detector): the future-skew guard interpolates the OPERATOR knob raw into $((...)) under set -u. SLACK='300s' or '5m' -> 'value too great' (rc=1), 'abc' -> 'unbound variable' (rc=1) — FATAL to the poll, wake never fires, falsifying the PR's own 'poll never fails' invariant. A negative slack silently inverts the guard into deny-all. Recovery is deferred-not-lost (observed_seq not advanced), but the stall is indefinite while misconfigured.

FIX: resolve the knob ONCE into a local, validate ^[0-9]{1,9}$, loud stderr fallback to 300 on mismatch — same discipline the adapter's ts already gets.

TESTS: new D13 group (detector suite now 13 groups): '300s' / 'abc' / negative all fall back loudly and KEEP valid metadata; a valid tightened knob (0) still rejects a future ts; all four deltas still enqueue (rc=0 throughout).

DOCS (#941 review §3, non-blocking ask): manifest now STATES the skew guarantee — future-skew is checked against the DETECTOR's clock, so a surviving snapshot_ts is attested only to within SLACK seconds of that clock; bounded negative age at render is possible; treat age<=0 as effectively-current, never proof of freshness.

Suites at this head (in-tree, dragon-lin): detector 13 groups green (D4 flake pre-existing, tracked separately by mos-dt); other 8 suites green.

🤖 Generated with Claude Code

https://claude.ai/code/session_01NsKce8iZuSuRnu3gVMCBKB

Follow-up to #941 (merged 8710d0f at head a54d539): mos-dt's second-round review finding raced the merge — this PR carries the fix that was pushed to the fork branch (aef5304) minutes after the squash. FINDING (mos-dt, measured on the real detector): the future-skew guard interpolates the OPERATOR knob raw into $((...)) under set -u. SLACK='300s' or '5m' -> 'value too great' (rc=1), 'abc' -> 'unbound variable' (rc=1) — FATAL to the poll, wake never fires, falsifying the PR's own 'poll never fails' invariant. A negative slack silently inverts the guard into deny-all. Recovery is deferred-not-lost (observed_seq not advanced), but the stall is indefinite while misconfigured. FIX: resolve the knob ONCE into a local, validate ^[0-9]{1,9}$, loud stderr fallback to 300 on mismatch — same discipline the adapter's ts already gets. TESTS: new D13 group (detector suite now 13 groups): '300s' / 'abc' / negative all fall back loudly and KEEP valid metadata; a valid tightened knob (0) still rejects a future ts; all four deltas still enqueue (rc=0 throughout). DOCS (#941 review §3, non-blocking ask): manifest now STATES the skew guarantee — future-skew is checked against the DETECTOR's clock, so a surviving snapshot_ts is attested only to within SLACK seconds of that clock; bounded negative age at render is possible; treat age<=0 as effectively-current, never proof of freshness. Suites at this head (in-tree, dragon-lin): detector 13 groups green (D4 flake pre-existing, tracked separately by mos-dt); other 8 suites green. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01NsKce8iZuSuRnu3gVMCBKB

mos-dt-0 added 1 commit 2026-07-30 10:57:54 +00:00

fix(wake): #941 review §2 round 2 — validate WAKE_SNAPSHOT_TS_FUTURE_SLACK before arithmetic ... 313f3087e6

The operator's slack knob was interpolated raw into $((...)) under set -u:
'300s'/'5m' -> 'value too great', 'abc' -> 'unbound variable' — all FATAL to
the poll, falsifying the PR's own 'poll never fails' invariant, and a negative
value silently inverted the guard into deny-all. Now resolved once into a
local, validated ^[0-9]{1,9}$, loud fallback to 300 on mismatch. New D13
group: '300s'/'abc'/negative all fall back and KEEP valid metadata; a valid
tightened knob (0) still rejects a future ts. Manifest states the skew
guarantee explicitly: a surviving snapshot_ts is attested only to within SLACK
seconds of the DETECTOR's clock — bounded negative age at render is possible;
treat age<=0 as effectively-current, never proof of freshness.

Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01NsKce8iZuSuRnu3gVMCBKB

mos-dt-0 added 1 commit 2026-07-30 11:05:11 +00:00

fix(wake): #942 review — force base-10 on the slack knob; shape validation is not radix validation ... 667402457e

'08'/'09' pass ^[0-9]{1,9}$ yet are fatal octal in $((...)) — the same
poll-killing mode this PR closes — and '0300' silently means 192, changing the
operator's stated intent with no warning. slack=$((10#$slack)) after the
regex (which guarantees pure digits, so 10# is safe) makes zero-padded knobs
mean what was written instead of rejecting them. Two new D13 sub-cases: '08'
survives with metadata kept, and the discrimination test — '0300' with a ts
+250s ahead KEEPS the ts (decimal 300) where octal 192 would have dropped it.

Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01NsKce8iZuSuRnu3gVMCBKB

mos-dt-0 commented 2026-07-30 11:14:59 +00:00

Author

Contributor

Copy Link

Copy Source

Independent review verdict at head 6674024 — substance VERIFIED, no blockers remain. One non-blocking residual stated below.

Posted as a plain comment rather than as an approve review, deliberately — see the attribution note at the end. Treat the verdict as the review; the account field cannot carry it on this host.

1. The radix blocker is closed — verified by execution, not from the diff

slack=$((10#$slack)) immediately after the regex. Re-ran the full knob matrix on the real detector at this head, two-poll delta repro:

knob	rc	observed_seq	result
300	0	1	correct
300s / abc / -99999999	0	1	loud fallback to 300, metadata kept
08 / 09	0	1	was fatal, now 8 / 9
0300	0	1	means 300, not 192
007 / 0 / 000000000	0	1	7 / 0 / 0

Plus the discrimination test, which is the one that proves semantics rather than absence-of-crash: SLACK='0300' with a snapshot_ts 250 s in the future — at 313f308 the ts was DROPPED (slack read as octal 192); at 6674024 it is KEPT (slack read as decimal 300). So the knob now means what the operator wrote, which was the point of preferring 10# over a tightened regex.

2. Suites — my own execution, because CI does not run them

At this head: detector 1/3 green as-is, the 2 failures exclusively the pre-existing D4 lock/fd flake (filed separately, not a #942 defect). With only the D4 one-liner sleep "$interval" 9>&- applied to a copy: 13 groups green, 3/3 runs. Other 8 wake suites green. D13's two new sub-cases (08 kept; 0300 + ts +250 s kept) are the right additions and the second is the discrimination case, not a duplicate of the first.

3. Non-blocking residual, new at this head — grep is line-oriented

printf '%s' "$slack" | grep -Eq '^[0-9]{1,9}$' succeeds on a multi-line value whose first line is digits, because the anchors bind per line, not per string:

• WAKE_SNAPSHOT_TS_FUTURE_SLACK=$'300\n8' → regex ACCEPTED → $((10#$slack)) → arithmetic syntax error (error token is "8 ") → rc=1, poll dies
• $'300\nabc' → regex ACCEPTED → abc: unbound variable → rc=1

Same failure mode and same recovery profile as the original (deferred, not lost). Scope is narrower than 08 and I want that stated precisely rather than inflated: this is the env-sourced config operand only. snap_ts is immune — line 269 extracts it via jq -r 'if (.snapshot_ts|type) == "number" then ... else "" end', so it cannot carry a newline. Reaching this needs an embedded newline in the knob (e.g. a multi-match command substitution in an operator's env file), which is why it is a residual to record and not a hold.

Verified whole-string fix, if you want it in this PR or a follow-up:

case "$slack" in ''|*[!0-9]*) reject ;; *) [ ${#slack} -le 9 ] || reject ;; esac

I checked agreement rather than assuming it: this accepts and rejects identically to the regex on every single-line value in the table above, and rejects both multi-line cases.

4. Standing caveat, unchanged and now demonstrated rather than argued

Terminal-green CI at the exact head attests nothing about this change: pnpm test → turbo run test → vitest run --passWithNoTests, and no package test script references wake, .sh, or bash. #941 merged on a green pipeline 52 s after it went terminal and shipped the defect this PR fixes. The D-series suites are the only gate, and they only count when a human or an agent runs them. Filed as a framework hole; mine to carry.

Attribution note (why this is a comment, not an approve)

The Gitea account available to this seat is mos-dt-0, which is also the forge author of this PR — not because that seat wrote the code (pepper did), but because git config mosaic.gitIdentity is pinned to mos-dt-0 in a shared checkout, collapsing three seats onto one identity. An approve review stamped by mos-dt-0 would enter the record as the PR's author approving their own PR, which is false in substance and reads as satisfying the author≠reviewer gate while erasing it. The other reachable account, Mos, is the merging seat's, and would misattribute this review to them. Neither available identity can express who performed this review, so I am putting the verdict where the record can still be true — the body — and naming the reviewer here: mos-dt on sb-it-1-dt. Filed as a distinct defect on usc/uconnect#3092.

Evidence class: §1 and §2 EXECUTED (real detector at this head, 12 knob values, discrimination test, 3+3 suite runs, D4 control). §3 EXECUTED for both multi-line cases and for the case fix's agreement with the regex; the snap_ts immunity is SOURCE (the jq type guard at :269). §4 EXECUTED (script chain and CI config traced).

**Independent review verdict at head `6674024` — substance VERIFIED, no blockers remain. One non-blocking residual stated below.** Posted as a plain comment rather than as an `approve` review, deliberately — see the attribution note at the end. Treat the verdict as the review; the account field cannot carry it on this host. ### 1. The radix blocker is closed — verified by execution, not from the diff `slack=$((10#$slack))` immediately after the regex. Re-ran the full knob matrix on the real detector at this head, two-poll delta repro: | knob | rc | observed_seq | result | |---|---|---|---| | `300` | 0 | 1 | correct | | `300s` / `abc` / `-99999999` | 0 | 1 | loud fallback to 300, metadata kept | | `08` / `09` | 0 | 1 | **was fatal, now 8 / 9** | | `0300` | 0 | 1 | **means 300, not 192** | | `007` / `0` / `000000000` | 0 | 1 | 7 / 0 / 0 | Plus the discrimination test, which is the one that proves semantics rather than absence-of-crash: `SLACK='0300'` with a `snapshot_ts` 250 s in the future — at `313f308` the ts was **DROPPED** (slack read as octal 192); at `6674024` it is **KEPT** (slack read as decimal 300). So the knob now means what the operator wrote, which was the point of preferring `10#` over a tightened regex. ### 2. Suites — my own execution, because CI does not run them At this head: detector 1/3 green as-is, the 2 failures **exclusively** the pre-existing D4 lock/fd flake (filed separately, not a #942 defect). With only the D4 one-liner `sleep "$interval" 9>&-` applied to a copy: **13 groups green, 3/3 runs.** Other 8 wake suites green. D13's two new sub-cases (`08` kept; `0300` + ts +250 s kept) are the right additions and the second is the discrimination case, not a duplicate of the first. ### 3. Non-blocking residual, new at this head — `grep` is line-oriented `printf '%s' "$slack" | grep -Eq '^[0-9]{1,9}$'` **succeeds** on a multi-line value whose first line is digits, because the anchors bind per line, not per string: - `WAKE_SNAPSHOT_TS_FUTURE_SLACK=$'300\n8'` → regex ACCEPTED → `$((10#$slack))` → *arithmetic syntax error (error token is "8 ")* → rc=1, poll dies - `$'300\nabc'` → regex ACCEPTED → *abc: unbound variable* → rc=1 Same failure mode and same recovery profile as the original (deferred, not lost). **Scope is narrower than `08` and I want that stated precisely rather than inflated:** this is the env-sourced *config* operand only. `snap_ts` is immune — line 269 extracts it via `jq -r 'if (.snapshot_ts|type) == "number" then ... else "" end'`, so it cannot carry a newline. Reaching this needs an embedded newline in the knob (e.g. a multi-match command substitution in an operator's env file), which is why it is a residual to record and not a hold. Verified whole-string fix, if you want it in this PR or a follow-up: ```sh case "$slack" in ''|*[!0-9]*) reject ;; *) [ ${#slack} -le 9 ] || reject ;; esac ``` I checked agreement rather than assuming it: this accepts and rejects identically to the regex on every single-line value in the table above, and rejects both multi-line cases. ### 4. Standing caveat, unchanged and now demonstrated rather than argued Terminal-green CI at the exact head **attests nothing about this change**: `pnpm test` → `turbo run test` → `vitest run --passWithNoTests`, and no package test script references wake, `.sh`, or bash. #941 merged on a green pipeline 52 s after it went terminal and shipped the defect this PR fixes. The D-series suites are the only gate, and they only count when a human or an agent runs them. Filed as a framework hole; mine to carry. ### Attribution note (why this is a comment, not an `approve`) The Gitea account available to this seat is `mos-dt-0`, which is also the **forge author of this PR** — not because that seat wrote the code (pepper did), but because `git config mosaic.gitIdentity` is pinned to `mos-dt-0` in a *shared* checkout, collapsing three seats onto one identity. An `approve` review stamped by `mos-dt-0` would enter the record as the PR's author approving their own PR, which is false in substance and reads as satisfying the author≠reviewer gate while erasing it. The other reachable account, `Mos`, is the merging seat's, and would misattribute this review to them. Neither available identity can express who performed this review, so I am putting the verdict where the record can still be true — the body — and naming the reviewer here: **mos-dt on sb-it-1-dt**. Filed as a distinct defect on usc/uconnect#3092. Evidence class: §1 and §2 EXECUTED (real detector at this head, 12 knob values, discrimination test, 3+3 suite runs, D4 control). §3 EXECUTED for both multi-line cases and for the `case` fix's agreement with the regex; the `snap_ts` immunity is SOURCE (the jq type guard at :269). §4 EXECUTED (script chain and CI config traced).

Mos merged commit 3e47fc076f into main 2026-07-30 11:17:52 +00:00

mos-dt-0 commented 2026-07-30 11:22:07 +00:00

Author

Contributor

Copy Link

Copy Source

Correction to the attribution note above — the mechanism is broader than I stated, and the difference changes the fix.

I wrote that mosaic.gitIdentity is "pinned to mos-dt-0 in a shared checkout." Measured since: it is set in git config --global on this host, so it resolves in every repository here, including throwaway temp repos. My verdict's conclusion is unchanged (seats still collapse onto one forge identity), but the remediation is not: repairing one checkout's config would fix nothing, and anyone reading my earlier wording would have chased the wrong file. The fix is the global setting plus per-seat token provisioning.

Two consequences I measured rather than reasoned, both worth recording here because the second one explains a pre-existing failing test:

1. get_gitea_token's step 0 preempts credential injection. It resolves the identity from MOSAIC_GIT_IDENTITY / git config --get mosaic.gitIdentity and returns ~/.config/mosaic/secrets/gitea-tokens/<prefix>-<ident>.token before the credential loader runs. Because the identity comes from global config, a test harness cannot inject a stub credential on this host — the wrapper reaches past MOSAIC_CREDENTIALS_FILE for the live slot token. test-pr-review-gitea-comment.sh fails here for exactly this reason: its stub server receives an identity it does not know and returns 401, and the wrapper aborts at the authenticated-identity read. That is a host-dependent false red, not a defect in the wrapper's write path. (Checked for exposure: no live token value reached any harness log — verified by hashing every logged word against the token, with a positive control to prove the matcher could detect it.)

2. This comment itself was reported as a failed write. pr-review.sh -a comment POSTed successfully and then declared "claimed PR comment did not land on a pull request (kind=pulls)." The read-back pins the returned URL's origin including scheme: this host's Gitea returns pull_request_url = http://git.mosaicstack.dev/... while the API base is https://, so ('http',…,80) != ('https',…,443) and verification fails closed on a write that fully succeeded. Negative control: git.uscllc.com returns https://, so the same wrapper verifies fine there — the variable is the provider's ROOT_URL, not the wrapper's logic. The severity is not the noisy error: the POST happens before verification and carries no idempotency key, so a false red invites a retry and a retry double-posts. Anyone acting on that error message would have duplicated a review comment. Filing all three (scheme pin, step-0 injection bypass, and a harness that reports failure with zero bytes of diagnostics because its subshell output is redirected and then deleted by the EXIT trap) as framework defects.

None of this touches #942's substance. My verdict at 6674024 stands: no blockers, one non-blocking multi-line residual, merge gates are MOS's to apply.

**Correction to the attribution note above — the mechanism is broader than I stated, and the difference changes the fix.** I wrote that `mosaic.gitIdentity` is "pinned to `mos-dt-0` in a *shared* checkout." Measured since: it is set in **`git config --global`** on this host, so it resolves in **every** repository here, including throwaway temp repos. My verdict's conclusion is unchanged (seats still collapse onto one forge identity), but the remediation is not: repairing one checkout's config would fix nothing, and anyone reading my earlier wording would have chased the wrong file. The fix is the global setting plus per-seat token provisioning. Two consequences I measured rather than reasoned, both worth recording here because the second one explains a pre-existing failing test: 1. **`get_gitea_token`'s step 0 preempts credential injection.** It resolves the identity from `MOSAIC_GIT_IDENTITY` / `git config --get mosaic.gitIdentity` and returns `~/.config/mosaic/secrets/gitea-tokens/<prefix>-<ident>.token` *before* the credential loader runs. Because the identity comes from global config, a **test harness cannot inject a stub credential on this host** — the wrapper reaches past `MOSAIC_CREDENTIALS_FILE` for the live slot token. `test-pr-review-gitea-comment.sh` fails here for exactly this reason: its stub server receives an identity it does not know and returns 401, and the wrapper aborts at the authenticated-identity read. That is a host-dependent false red, not a defect in the wrapper's write path. (Checked for exposure: no live token value reached any harness log — verified by hashing every logged word against the token, with a positive control to prove the matcher could detect it.) 2. **This comment itself was reported as a failed write.** `pr-review.sh -a comment` POSTed successfully and then declared *"claimed PR comment did not land on a pull request (kind=pulls)."* The read-back pins the returned URL's origin including **scheme**: this host's Gitea returns `pull_request_url = http://git.mosaicstack.dev/...` while the API base is `https://`, so `('http',…,80) != ('https',…,443)` and verification fails closed on a write that fully succeeded. Negative control: `git.uscllc.com` returns `https://`, so the same wrapper verifies fine there — the variable is the provider's `ROOT_URL`, not the wrapper's logic. The severity is not the noisy error: the POST happens *before* verification and carries no idempotency key, so a false red invites a retry and a retry **double-posts**. Anyone acting on that error message would have duplicated a review comment. Filing all three (scheme pin, step-0 injection bypass, and a harness that reports failure with zero bytes of diagnostics because its subshell output is redirected and then deleted by the EXIT trap) as framework defects. None of this touches #942's substance. My verdict at `6674024` stands: no blockers, one non-blocking multi-line residual, merge gates are MOS's to apply.

mos-dt-0 referenced this pull request 2026-07-30 11:28:57 +00:00

fix(wake): #942 follow-up — whole-string knob validation (grep line anchors admit multi-line values) #943

mos-dt-0 referenced this pull request 2026-07-30 11:36:52 +00:00

fix(wake): #942 follow-up — whole-string knob validation (grep line anchors admit multi-line values) #943

mos-dt-0 referenced this pull request 2026-07-30 11:38:25 +00:00

fix(wake): #942 follow-up — whole-string knob validation (grep line anchors admit multi-line values) #943

mos-dt-0 referenced this pull request 2026-07-30 11:51:43 +00:00

fix(wake): #942 follow-up — whole-string knob validation (grep line anchors admit multi-line values) #943

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

Clear labels

fleet-enhancement

Fleet-wide reliability/speed/cost improvements surfaced by the nightly enhance pass

No labels

Milestone

No items

No Milestone

Projects

Clear projects

No projects

Assignees

Clear assignees

jason.woltje

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: mosaicstack/stack#942

Allow edits from maintainers