Post-#149 review pass 4: 1 medium + 2 low findings (asymmetric migration, script hygiene)

Brouie commented

2026-05-05 04:59:20 +00:00

(Migrated from gitlab.com)

fourth-pass review on the broader stack at HEAD e12ddab, dedup'd against everything filed today (#148 closed, #149 in flight with finding 2 added today as note, #150 MR !374 pending merge). three real findings, all hand-verified.

a fourth finding (second instance of #149 finding 4 LOWER(...) index defeat on timecurve_warbow_steals_by_victim_day) was flagged inline on #149 since the fix is identical to that ticket's existing patch.

ordered by severity descending. finding 1 is a deploy / rollback hygiene gap worth pre-mainnet attention; findings 2 and 3 are post-deploy hardening.

Finding 1 — Indexer: `20260504180000_timecurve_unredeemed_launched_token` migration ships .up.sql with no .down.sql (Medium)

indexer/migrations/ listing:

ls indexer/migrations/ | grep 20260504180000
20260504180000_timecurve_unredeemed_launched_token.up.sql

every other migration in the directory ships a paired .down.sql (verified spot-checks). this one shipped .up only.

concrete consequence: rollback of this schema step (the unredeemed-launched-token table introduced for the #128 sweep flow) cannot run via the migration runner. an operator forced to revert post-deploy must drop the table by hand, and the migrator's _sqlx_migrations version table will not be reconciled — blocking automated re-application of the same version. on a TGE-bound deploy with an active rollback plan this is a failure mode worth closing pre-mainnet.

mitigation: add 20260504180000_timecurve_unredeemed_launched_token.down.sql containing the inverse DROP TABLE IF EXISTS … (and any related index drops) so the pair is symmetric with the rest of the migration set.

Finding 2 — Script: `start-qa-local-full-stack.sh` background Vite has no trap; orphans on orchestrator death (Low)

scripts/start-qa-local-full-stack.sh:115-128:

echo "=== Vite dev (127.0.0.1:${port}) ===" >&2
(
  cd "${FRONTEND}"
  npm run dev -- --host 127.0.0.1 --port "${port}"
) >>"${log_file}" 2>&1 &
echo $! >"${pid_file}"
echo "  Log: ${log_file}   PID: $(cat "${pid_file}")" >&2
for _ in $(seq 1 60); do
  if curl -sf "http://127.0.0.1:${port}/" >/dev/null 2>&1; then
    break
  fi
  sleep 0.5
done

backgrounds npm run dev, writes PID — but no trap … EXIT INT TERM upstream to kill it on script abort.

concrete failure mode: if the orchestrator dies or the user Ctrl-Cs after the readiness loop has begun, the Vite child keeps holding ${FRONTEND_DEV_PORT:-5173}. next start-qa-local-full-stack.sh invocation either:

fails to bind 5173 (clean failure, easy to diagnose)
silently passes the readiness curl probe at L122-127 against the orphan from the previous run (silent failure — Vite serves stale code from the previous run while the operator thinks they're testing fresh)

I've actually hit the silent-orphan version of this today during the #150 stack bring-up — anvil orphan held 8545 from a previous failed run and the second attempt's readiness probe falsely passed against it. same shape applies to Vite here.

mitigation: add a trap inside the launching function before the background invocation:

trap 'kill "$(cat "${pid_file}" 2>/dev/null)" 2>/dev/null || true' EXIT INT TERM

or have the readiness loop fail-closed if curl ever returned 0 BEFORE the loop's first sleep (catches stale-orphan masquerading as fresh-ready).

Finding 3 — Library: `_yieldomega_env_set_line` sed escaping is fragile (Low — preventive)

scripts/lib/kumbaya_local_anvil_env.sh:37-52:

# Idempotent: set or replace KEY=value (hex addresses — avoid special chars in values).
_yieldomega_env_set_line() {
  local file="$1" key="$2" val="$3"
  ...
  if grep -q "^${key}=" "${file}" 2>/dev/null; then
    sed -i "s#^${key}=.*#${key}=${val}#" "${file}"

${key} is interpolated unescaped into the regex side and ${val} is interpolated unescaped into the replacement side of the sed command. header at L37 acknowledges "hex addresses — avoid special chars in values".

today's callers (VITE_KUMBAYA_* keys, 0x-prefixed addresses) are safe. but the helper is reused via:

yieldomega_frontend_merge_kumbaya_vite_full
yieldomega_frontend_merge_vite_kumbaya_buy_router_only

the moment any future caller passes a value containing &, \, or # (path separators, signed payloads, base64 with / in it, etc.), the replacement becomes silently wrong (& re-injects the matched LHS, # ends the s-expression early, \ injects backreferences). silent corruption rather than loud failure.

mitigation: pre-escape both sides:

local k_esc v_esc
k_esc="$(printf '%s' "${key}" | sed -e 's/[][\.*^$\/]/\\&/g')"
v_esc="$(printf '%s' "${val}" | sed -e 's/[\&\/]/\\&/g')"
sed -i "s#^${k_esc}=.*#${k_esc}=${v_esc}#" "${file}"

or switch to a different in-place editor (awk, python, dedicated env-file lib) that doesn't treat values as regex. preventive — no exploit today, but the moment someone adds a key with a non-trivial value the breakage will be silent.

Pre-deploy relevance

deploy-tonight scope: finding 1 (rollback safety pre-mainnet).
post-deploy hardening: findings 2, 3.

cc @PlasticDigits

fourth-pass review on the broader stack at HEAD `e12ddab`, dedup'd against everything filed today (#148 closed, #149 in flight with finding 2 added today as note, #150 MR !374 pending merge). three real findings, all hand-verified. a fourth finding (second instance of #149 finding 4 `LOWER(...)` index defeat on `timecurve_warbow_steals_by_victim_day`) was flagged inline on #149 since the fix is identical to that ticket's existing patch. ordered by severity descending. finding 1 is a deploy / rollback hygiene gap worth pre-mainnet attention; findings 2 and 3 are post-deploy hardening. --- ## Finding 1 — Indexer: `20260504180000_timecurve_unredeemed_launched_token` migration ships .up.sql with no .down.sql (Medium) `indexer/migrations/` listing: ``` ls indexer/migrations/ | grep 20260504180000 20260504180000_timecurve_unredeemed_launched_token.up.sql ``` every other migration in the directory ships a paired `.down.sql` (verified spot-checks). this one shipped `.up` only. **concrete consequence**: rollback of this schema step (the unredeemed-launched-token table introduced for the #128 sweep flow) cannot run via the migration runner. an operator forced to revert post-deploy must drop the table by hand, and the migrator's `_sqlx_migrations` version table will not be reconciled — blocking automated re-application of the same version. on a TGE-bound deploy with an active rollback plan this is a failure mode worth closing pre-mainnet. **mitigation**: add `20260504180000_timecurve_unredeemed_launched_token.down.sql` containing the inverse `DROP TABLE IF EXISTS …` (and any related index drops) so the pair is symmetric with the rest of the migration set. --- ## Finding 2 — Script: `start-qa-local-full-stack.sh` background Vite has no trap; orphans on orchestrator death (Low) `scripts/start-qa-local-full-stack.sh:115-128`: ``` echo "=== Vite dev (127.0.0.1:${port}) ===" >&2 ( cd "${FRONTEND}" npm run dev -- --host 127.0.0.1 --port "${port}" ) >>"${log_file}" 2>&1 & echo $! >"${pid_file}" echo " Log: ${log_file} PID: $(cat "${pid_file}")" >&2 for _ in $(seq 1 60); do if curl -sf "http://127.0.0.1:${port}/" >/dev/null 2>&1; then break fi sleep 0.5 done ``` backgrounds `npm run dev`, writes PID — but no `trap … EXIT INT TERM` upstream to kill it on script abort. **concrete failure mode**: if the orchestrator dies or the user Ctrl-Cs after the readiness loop has begun, the Vite child keeps holding `${FRONTEND_DEV_PORT:-5173}`. next `start-qa-local-full-stack.sh` invocation either: - fails to bind 5173 (clean failure, easy to diagnose) - silently passes the readiness curl probe at L122-127 against the orphan from the previous run (silent failure — Vite serves stale code from the previous run while the operator thinks they're testing fresh) I've actually hit the silent-orphan version of this today during the #150 stack bring-up — anvil orphan held 8545 from a previous failed run and the second attempt's readiness probe falsely passed against it. same shape applies to Vite here. **mitigation**: add a trap inside the launching function before the background invocation: ```bash trap 'kill "$(cat "${pid_file}" 2>/dev/null)" 2>/dev/null || true' EXIT INT TERM ``` or have the readiness loop fail-closed if curl ever returned 0 BEFORE the loop's first sleep (catches stale-orphan masquerading as fresh-ready). --- ## Finding 3 — Library: `_yieldomega_env_set_line` sed escaping is fragile (Low — preventive) `scripts/lib/kumbaya_local_anvil_env.sh:37-52`: ``` # Idempotent: set or replace KEY=value (hex addresses — avoid special chars in values). _yieldomega_env_set_line() { local file="$1" key="$2" val="$3" ... if grep -q "^${key}=" "${file}" 2>/dev/null; then sed -i "s#^${key}=.*#${key}=${val}#" "${file}" ``` `${key}` is interpolated unescaped into the regex side and `${val}` is interpolated unescaped into the replacement side of the `sed` command. header at L37 acknowledges "hex addresses — avoid special chars in values". today's callers (`VITE_KUMBAYA_*` keys, `0x`-prefixed addresses) are safe. but the helper is reused via: - `yieldomega_frontend_merge_kumbaya_vite_full` - `yieldomega_frontend_merge_vite_kumbaya_buy_router_only` the moment any future caller passes a value containing `&`, `\`, or `#` (path separators, signed payloads, base64 with `/` in it, etc.), the replacement becomes silently wrong (`&` re-injects the matched LHS, `#` ends the s-expression early, `\` injects backreferences). silent corruption rather than loud failure. **mitigation**: pre-escape both sides: ```bash local k_esc v_esc k_esc="$(printf '%s' "${key}" | sed -e 's/[][\.*^$\/]/\\&/g')" v_esc="$(printf '%s' "${val}" | sed -e 's/[\&\/]/\\&/g')" sed -i "s#^${k_esc}=.*#${k_esc}=${v_esc}#" "${file}" ``` or switch to a different in-place editor (awk, python, dedicated env-file lib) that doesn't treat values as regex. preventive — no exploit today, but the moment someone adds a key with a non-trivial value the breakage will be silent. --- ## Pre-deploy relevance - **deploy-tonight scope**: finding 1 (rollback safety pre-mainnet). - **post-deploy hardening**: findings 2, 3. cc @PlasticDigits

PlasticDigits commented

2026-05-05 05:45:38 +00:00

(Migrated from gitlab.com)

mentioned in issue #152

PlasticDigits commented

2026-05-05 05:45:39 +00:00

(Migrated from gitlab.com)

marked this issue as related to #152

PlasticDigits commented

2026-05-05 05:45:53 +00:00

(Migrated from gitlab.com)

mentioned in issue #153

PlasticDigits commented

2026-05-05 05:45:54 +00:00

(Migrated from gitlab.com)

marked this issue as related to #153

PlasticDigits commented

2026-05-05 05:45:58 +00:00

(Migrated from gitlab.com)

mentioned in issue #154

PlasticDigits commented

2026-05-05 05:45:59 +00:00

(Migrated from gitlab.com)

marked this issue as related to #154

PlasticDigits commented

2026-05-05 05:46:11 +00:00

(Migrated from gitlab.com)

Split into tracked follow-ups (verified still present at main HEAD): #152 migration .down.sql, #153 Vite orphan/trap in start-qa-local-full-stack.sh, #154 _yieldomega_env_set_line sed hardening. The duplicate LOWER(...) index note remains on #149 per inline comment.

Split into tracked follow-ups (verified still present at `main` HEAD): [#152](https://gitlab.com/PlasticDigits/yieldomega/-/issues/152) migration `.down.sql`, [#153](https://gitlab.com/PlasticDigits/yieldomega/-/issues/153) Vite orphan/trap in `start-qa-local-full-stack.sh`, [#154](https://gitlab.com/PlasticDigits/yieldomega/-/issues/154) `_yieldomega_env_set_line` sed hardening. The duplicate `LOWER(...)` index note remains on [#149](https://gitlab.com/PlasticDigits/yieldomega/-/issues/149) per inline comment.

Brouie commented

2026-05-07 04:50:13 +00:00

(Migrated from gitlab.com)

@PlasticDigits — closing this umbrella now that all 3 child tickets shipped:

#152 (migration .down.sql) ✓ closed
#153 (Vite trap in orchestrator) ✓ closed
#154 (sed hardening) ✓ closed

no remaining work on the parent.

@PlasticDigits — closing this umbrella now that all 3 child tickets shipped: - #152 (migration `.down.sql`) ✓ closed - #153 (Vite trap in orchestrator) ✓ closed - #154 (sed hardening) ✓ closed no remaining work on the parent.

Brouie (Migrated from gitlab.com) closed this issue

2026-05-07 04:50:14 +00:00

Rows
Columns

Post-#149 review pass 4: 1 medium + 2 low findings (asymmetric migration, script hygiene) #151

Finding 1 — Indexer: 20260504180000_timecurve_unredeemed_launched_token migration ships .up.sql with no .down.sql (Medium)

Finding 2 — Script: start-qa-local-full-stack.sh background Vite has no trap; orphans on orchestrator death (Low)

Finding 3 — Library: _yieldomega_env_set_line sed escaping is fragile (Low — preventive)

Pre-deploy relevance

Finding 1 — Indexer: `20260504180000_timecurve_unredeemed_launched_token` migration ships .up.sql with no .down.sql (Medium)

Finding 2 — Script: `start-qa-local-full-stack.sh` background Vite has no trap; orphans on orchestrator death (Low)

Finding 3 — Library: `_yieldomega_env_set_line` sed escaping is fragile (Low — preventive)