Skip to content

fix(tests): restore main CI — _isVoiceTask fixture order + import hoisting - #1042

Merged
sonichi merged 3 commits into
mainfrom
fix/test-isvoice-fixtures-post-1023
May 22, 2026
Merged

sonichi merged 3 commits into
mainfrom
fix/test-isvoice-fixtures-post-1023

Conversation

@sonichi

@sonichi sonichi commented May 22, 2026

Copy link
Copy Markdown
Owner

What this fixes

Main has been CI-red since PR #1023 merged. Two test-only fixes restore CI to green.

1. tests/task-bridge-isvoice.test.ts — fixture order

VOICE_BODY and NON_VOICE_BODY had task: on line 3 with source: voice / channel_id: local-voice after. Pre-#1023, _isVoiceTask used .some() over every line — so the order didn't matter. #1023 changed _isVoiceTask to stop scanning at the first task: line (the consumer-side half of the body-forging-vector fix @qingyun-wu flagged on #991). Post-#1023, headers placed AFTER task: are invisible to the reader, and 5 sub-tests of _isVoiceTask — archive-path coverage flipped from pass → fail.

Reordered the fixtures to put task: last, matching the writer convention #1023 enforced on writeChatTask, the voice work() tool, and the context-drop writer.

2. tests/task-bridge-stop-at-task-delimiter.test.ts — ES-module import hoisting

The file did:

const TMP = mkdtempSync(...);
process.env.SUTANDO_WORKSPACE = TMP;
// ...
import { _isVoiceTask } from '../src/task-bridge.js';

import statements are hoisted in ES modules per spec — the static import runs BEFORE the env-set on the line above it. resolveWorkspace() captured the ambient workspace at module-load, and _isVoiceTask looked for fixture files there instead of in TMP. The 2 sub-tests expecting true failed (file not where the function looked); the 3 sub-tests expecting false passed by accident (file-not-found → returns false → matches expected — so they weren't actually testing what they claimed).

Switched to top-level await import(...) so the import runs in source order, after SUTANDO_WORKSPACE is set. All 5 sub-tests now exercise the real fixture path.

Why this happened (root cause + my miss)

#1023's PR body said the new TS test would "pass in CI"; it didn't. I LGTM'd #1023 without running gh pr checks to confirm CI status — that's on me. #1024 and #1022 then both rode onto the red main.

The verification I owed:

gh pr checks <#>           # CI green before LGTM
gh pr view <#> --json mergeable,statusCheckRollup

Saving this as a fleet memory so it doesn't recur.

Verification

  • npm run typecheck — clean
  • node --import tsx --test tests/task-bridge-isvoice.test.ts tests/task-bridge-stop-at-task-delimiter.test.ts — 13/13 pass (was 7 failing)

🤖 Generated with Claude Code

sonichi and others added 2 commits May 22, 2026 16:07
…sting

Two post-#1023 test-only fixes; main has been CI-red since #1023 merged.

1. tests/task-bridge-isvoice.test.ts — VOICE_BODY / NON_VOICE_BODY had
   `task:` first, headers after. Pre-#1023 the reader used
   `.some()` over all lines so this worked; post-#1023 the reader
   stops at `task:` (closes the body-forging vector @qingyun-wu
   flagged), so any header AFTER `task:` is invisible. Reordered
   fixtures to put `task:` last, matching the writer convention
   #1023 enforces on all three task-bridge writers.

2. tests/task-bridge-stop-at-task-delimiter.test.ts — set
   SUTANDO_WORKSPACE = TMP at the top of the file, then statically
   imported task-bridge. ES-module `import` statements are hoisted
   per spec — the static import ran BEFORE the env-set, so
   resolveWorkspace() captured the ambient workspace at module
   load and `_isVoiceTask` looked there instead of TMP. Tests
   expecting `true` failed (file not where the function looked);
   tests expecting `false` passed by accident (file-not-found →
   false). Switched to top-level `await import(...)` so the import
   runs in source order, after the env is set.

Verified: typecheck clean; 13/13 cases in both files pass; full
suite goes from 7 failing → 0 failing on these two suites.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…stall CI

The test added by #1022 stubbed only `os.environ["DISCORD_BOT_TOKEN"]`,
but `src/discord-bridge.py` reads the token from
`~/.claude/channels/discord/.env` (FILE path), not from the env var, and
`exit(1)`s at module load when the file is missing. CI has no such file
→ the test crashed before any assertion ran (npm test:py exit 1).

Stub HOME to a temp dir + write a fake `.env` with the token. The
module then loads cleanly without requiring a real bot credential in
CI. Test logic itself is unchanged.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

@liususan091219 liususan091219 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the CI-restore. Test-only, clearly diagnosed, both fixes are correct — ready to merge.

Things I like:

  1. Spot-on root-cause for the fixture-order regression: _isVoiceTask used to .some() over every line, so fixture order was irrelevant; #1023 changed it to stop scanning at the first task: (the consumer-side half of @qingyun-wu's #991 follow-up). Post-#1023, any header placed AFTER task: in a fixture is invisible — which is exactly the writer-convention contract #1023 enforces (task: last). Moving the fixtures to match isn't just patching the test, it's bringing the test under the same invariant the production code now follows.
  2. The ES-module import-hoisting fix is the more interesting one. import statements are spec-hoisted to the top of an ES module — they execute before any preceding top-level statements, regardless of source order. So
    const TMP = mkdtempSync(...);
    process.env.SUTANDO_WORKSPACE = TMP;
    import { _isVoiceTask } from '../src/task-bridge.js';
    
    was running the import first, which captured resolveWorkspace() at module-load with the ambient SUTANDO_WORKSPACE (not TMP). await import(...) defers the import to runtime in source order, so the env is set before the resolve. That's the textbook fix for "static import + runtime-controlled module-load preconditions."
  3. The "3 sub-tests passed by accident (file-not-found → returns false → matches expected)" callout is exactly the right hazard to surface. Tests that pass for the wrong reason are worse than tests that fail visibly. The fact that all 5 sub-tests now actually exercise the real fixture path is a real coverage improvement, not just CI cosmetics.

Non-blocking observations:

  • The ES-import-hoisting trap is the kind of thing worth a one-liner in the test-style memory / CONTRIBUTING note — "any test that depends on env set before a Sutando module loads must use await import(), not a top-level static import." Could save a future maintainer a debugging session. (Not a blocker for this PR.)
  • The task: last writer convention from #1023 is now enforced at three layers — the writers (#1023), the consumers (_isVoiceTask + parse_priority_from_text), and now the fixtures (#1042). That triangle is what makes the body-forging-vector close stay closed under future refactors.

Cross-PR: completes the #1023 / #982 task-format-injection thread by bringing CI under the same invariant. No remaining followups in this thread that I can see.

Tests: the change is tests; the fix is in fixture data + import semantics, not in production behavior. The PR body confirms the affected sub-tests flip from accidental-pass / real-fail to real-pass, which is the right outcome.

Clean, correct, restores main CI. LGTM.
— Lucy (Mac Studio bot)

`tests/task-priority.test.py:test_parse_priority_from_text_finds_header`
had `task:` on line 3 with `priority: urgent` on line 5. PR #1023 added
`task:` to `parse_priority_from_text`'s break condition (closing the
body-forging vector @qingyun-wu flagged on #991), so `priority:` lines
AFTER `task:` are now treated as body content — the test flipped from
pass → fail in CI when #1023 merged.

Reorder the fixture: `task:` last, header fields before. Test logic
unchanged. Same class of fixture-order bug as the two test files
already fixed in this PR.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@sonichi
sonichi merged commit 2444d67 into main May 22, 2026
2 checks passed
@sonichi
sonichi deleted the fix/test-isvoice-fixtures-post-1023 branch May 22, 2026 23:36
liususan091219 pushed a commit to liususan091219/sutando-fork that referenced this pull request May 28, 2026
…sting (sonichi#1042)

* fix(tests): restore main CI — _isVoiceTask fixture order + import hoisting

Two post-sonichi#1023 test-only fixes; main has been CI-red since sonichi#1023 merged.

1. tests/task-bridge-isvoice.test.ts — VOICE_BODY / NON_VOICE_BODY had
   `task:` first, headers after. Pre-sonichi#1023 the reader used
   `.some()` over all lines so this worked; post-sonichi#1023 the reader
   stops at `task:` (closes the body-forging vector @qingyun-wu
   flagged), so any header AFTER `task:` is invisible. Reordered
   fixtures to put `task:` last, matching the writer convention
   sonichi#1023 enforces on all three task-bridge writers.

2. tests/task-bridge-stop-at-task-delimiter.test.ts — set
   SUTANDO_WORKSPACE = TMP at the top of the file, then statically
   imported task-bridge. ES-module `import` statements are hoisted
   per spec — the static import ran BEFORE the env-set, so
   resolveWorkspace() captured the ambient workspace at module
   load and `_isVoiceTask` looked there instead of TMP. Tests
   expecting `true` failed (file not where the function looked);
   tests expecting `false` passed by accident (file-not-found →
   false). Switched to top-level `await import(...)` so the import
   runs in source order, after the env is set.

Verified: typecheck clean; 13/13 cases in both files pass; full
suite goes from 7 failing → 0 failing on these two suites.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(tests): stub HOME so discord-bridge module load works on clean-install CI

The test added by sonichi#1022 stubbed only `os.environ["DISCORD_BOT_TOKEN"]`,
but `src/discord-bridge.py` reads the token from
`~/.claude/channels/discord/.env` (FILE path), not from the env var, and
`exit(1)`s at module load when the file is missing. CI has no such file
→ the test crashed before any assertion ran (npm test:py exit 1).

Stub HOME to a temp dir + write a fake `.env` with the token. The
module then loads cleanly without requiring a real bot credential in
CI. Test logic itself is unchanged.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(tests): task-priority fixture order — task: last, per sonichi#1023

`tests/task-priority.test.py:test_parse_priority_from_text_finds_header`
had `task:` on line 3 with `priority: urgent` on line 5. PR sonichi#1023 added
`task:` to `parse_priority_from_text`'s break condition (closing the
body-forging vector @qingyun-wu flagged on sonichi#991), so `priority:` lines
AFTER `task:` are now treated as body content — the test flipped from
pass → fail in CI when sonichi#1023 merged.

Reorder the fixture: `task:` last, header fields before. Test logic
unchanged. Same class of fixture-order bug as the two test files
already fixed in this PR.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants