Improve CI coverage and require green status - #121
Merged
Conversation
dylanneve1
force-pushed
the
improve-ci-functional-tests
branch
from
May 8, 2026 13:13
6157886 to
57fe7b5
Compare
claudiusthebot
approved these changes
May 8, 2026
claudiusthebot
left a comment
claudiusthebot
left a comment
Collaborator
There was a problem hiding this comment.
Clean improvement — 15/15 CI checks green, LGTM.
Key wins:
- Removes auto-format-and-push from CI. The old job held
contents: write+ pushed commits to PR branches on every format failure — a security risk (PAT in a bot-controlled path) and a review hygiene issue (CI could rewrite history mid-review). Dropping it is the right call; format check is now purely informational (continue-on-error: true). - macOS + Windows in the test matrix: catches cross-platform regressions that ubuntu-only would miss. The
package.functional.test.tstarball smoke test in particular —npm pack → npm install --ignore-scripts → talon --help— will catch missingfiles[]/.npmignoreentries that have bitten CLI tools before. vitest-summary.mjsis much cleaner than the shell heredocs it replaces. Reusable across all three test jobs, handles the coverage section conditionally without an extra step.enforce-ci-gate.mjs+branch-protection.jsongive a reproducible, declarative way to re-apply branch protection if it ever drifts (e.g. after a repo transfer or admin change).
One minor note: expectOk in package.functional.test.ts asserts stderr === "" strictly. This is fine in practice (CI passed on all 3 platforms) but Node.js can emit deprecation warnings to stderr on some minor versions — worth keeping an eye on if a future Node upgrade causes spurious failures. expectExitOk is the right fallback if that ever happens.
All 15 CI checks green. Good to merge.
The Code Quality job already hard-fails on tsc/lint/.only-grep, but format:check was kept informational while pre-existing Prettier warnings sat in the tree. The tree is now clean (verified locally), so flip the gate: a format failure now fails the quality job, which makes the CI Status aggregate fail, which blocks merge. knip stays informational by design (the step name says so) — strict knip would currently surface ~23 unused exports across opencode/core and would be a separate cleanup decision.
This was referenced May 8, 2026
dylanneve1
pushed a commit
that referenced
this pull request
May 9, 2026
…erRestart/store Adds targeted tests that bring branch coverage above the 60% global threshold imposed by PR #121. Exercises: commandForLanguage python & node paths, spawnTrigger idempotency guard, cancelTrigger false-return, resumeAfterRestart (no-deps / empty / matching / already-fired / old), trigger-store unknown-id paths, FIRE_PAYLOAD_MAX_BYTES truncation, and finalizeExit status-branch (cancelled exit + non-zero exit code). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
6 tasks
dylanneve1
pushed a commit
that referenced
this pull request
May 13, 2026
…erRestart/store Adds targeted tests that bring branch coverage above the 60% global threshold imposed by PR #121. Exercises: commandForLanguage python & node paths, spawnTrigger idempotency guard, cancelTrigger false-return, resumeAfterRestart (no-deps / empty / matching / already-fired / old), trigger-store unknown-id paths, FIRE_PAYLOAD_MAX_BYTES truncation, and finalizeExit status-branch (cancelled exit + non-zero exit code). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
dylanneve1
pushed a commit
that referenced
this pull request
May 13, 2026
* feat(triggers): self-authored watcher scripts that wake the bot
Triggers are bot-authored long-running scripts (bash / python / node)
that run as supervised subprocesses and signal back via stdout to fire
wake-up messages into the originating chat. Built for the case where a
fixed cron schedule doesn't fit — "wake me when this PR merges", "tell
me if BTC moves >5%", "watch this URL until it returns 200".
Contract:
- Mid-run: a stdout line `TALON_FIRE: <text>` fires immediately and the
script keeps running (multi-event watchers).
- Exit 0: final fire with the tail of stdout/stderr as payload.
- Exit non-zero: error fire with exit code + log tail.
- Hard timeout (default 24h, max 7d): SIGTERM → SIGKILL → timed_out fire.
Tools exposed: trigger_create, trigger_list, trigger_cancel,
trigger_logs, trigger_delete. Per-chat cap of 5 active. Children are
killed on Talon shutdown — they do not survive a restart, and any
trigger left in running/pending on load is reaped to "terminated".
New modules:
- src/storage/trigger-store.ts: persistence + validation
- src/core/triggers.ts: supervisor (spawn, line buffers, fire dispatch)
- src/core/tools/triggers.ts: 5 tool definitions
- gateway-actions.ts: trigger_* bridge handlers
- paths.ts: ~/.talon/data/trigger-runs/<chatId>/ for scripts and logs
- bootstrap.ts: loadTriggers + initTriggers + resumeAfterRestart
- index.ts: shutdownTriggers + flushTriggers in shutdown paths
Tests: 30 new (22 store + 8 supervisor including real-bash spawn
integration). 1397/1397 passing.
* style: auto-format with prettier
* test(triggers): branch coverage for python/node/idempotency/resumeAfterRestart/store
Adds targeted tests that bring branch coverage above the 60% global
threshold imposed by PR #121. Exercises: commandForLanguage python &
node paths, spawnTrigger idempotency guard, cancelTrigger false-return,
resumeAfterRestart (no-deps / empty / matching / already-fired / old),
trigger-store unknown-id paths, FIRE_PAYLOAD_MAX_BYTES truncation, and
finalizeExit status-branch (cancelled exit + non-zero exit code).
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* test(triggers): fix Windows path separator + add missing branch coverage
Two CI failures in the coverage-fix commit (9096518):
1. Windows N22 — trigger-store.test.ts: triggerScriptPath regex used Unix
forward-slashes but resolve() returns backslashes on Windows. Fixed by
normalising path separators before matching.
2. Ubuntu N22 — branch coverage 59.67% < 60% (threshold differs from N24
due to V8 v11 vs v12 branch instrumentation). Added 4 more targeted tests
in triggers-extended.test.ts:
- trigger-store persistNow() path (lines 159-160)
- readTriggerLogTail catch path via EISDIR (line 329)
- readTriggerLogTail truncated:true path (line 327)
- fireWakeUp dispatch-error catch path (line 394 of triggers.ts)
All 18 extended tests pass. typecheck ✅ · format:check ✅.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* test(triggers): add N22 branch coverage for TALON_FIRE, empty-output, and store helpers
Ubuntu Node 22 (V8 v11) instruments more branches than Node 24 (V8 v12),
causing coverage to land at 59.67% on N22 while passing on N24.
Added 7 new tests to triggers-extended.test.ts:
- Empty stdout: covers fireWake's `trimmed ? ... : (no output)` false branch
- Mid-run TALON_FIRE: covers handleStdoutLine true branch + fireWake
terminal=false ("signalled" header path)
- validateLanguage false paths: includes() false + typeof short-circuit
- sanitizeChatId with special chars: confirms replace() regex path
- languageExtension direct call: covers all 3 switch arms explicitly
- getTriggerByName returning undefined: covers find() undefined path
Also imports getTriggerByName, validateLanguage, sanitizeChatId, and
languageExtension from trigger-store for direct testing.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* test(triggers): comprehensive branch coverage round 4 — target Node 22 CI pass
Add 15 new tests across 4 test files to cover previously-unreachable branches,
specifically the ones V8 v11 (Node 22) counts but were uncovered since hb #179:
trigger-store.test.ts (+9 tests):
- loadTriggers: file-not-present, non-object JSON, corrupt primary (3 variants)
- Backup parses as null (line 94 ternary false arm)
- save() catch: Error throw and non-Error string throw (both instanceof arms)
- readTriggerLogTail: non-Error throw (false arm of err instanceof Error)
triggers-extended.test.ts (+5 new describe blocks):
- fireWake with undefined payload (payload ?? '' false arm)
- fireCount undefined treated as 0 (fireCount ?? 0 false arm)
- finalizeExit with null exit code — signal kill (code ?? undefined false arm)
- handleStdoutLine with no lineBuffer entry (pushBufferLine if(!buf) true arm)
- shutdownTriggers when no children running (if(children.size===0) true arm)
- Child process error event handler (line 152 handler covered)
- Timeout timer fires after child already exited (if(!c) return true arm)
cleanup-registry.test.ts (+1 test):
- Handler throws a non-Error string (err instanceof Error false arm)
mcp-launcher.test.ts (+1 test):
- ensureLauncher throws when file does not exist (if(!existsSync) true arm)
Uses vi.doMock to intercept node:fs before fresh module import.
Node 24 local result: 60.26% (2146/3561). Previous Node 22 CI: 59.73%.
1744 tests, all passing. tsc clean, prettier clean.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* fix(triggers): 5 Copilot review fixes — byte-safe truncation, persist on terminal, etc.
Address 5 of the 6 Copilot review threads on PR #96. The 6th (mid-run
TriggerStatus rename) is a larger semantic refactor and deferred to a
later commit; this commit pairs the smallest reasonable fix at each
site with regression tests where they're cleanly testable.
1. src/core/triggers.ts:128 — log WriteStream missing error handler.
createWriteStream() with no .on("error") emits an unhandled
EventEmitter `error` event if the log file becomes unwritable
(disk full, permission flip), which crashes the Node process. Added
a logError-routing handler so disk-tier failures degrade gracefully
without killing the supervisor.
2. src/storage/trigger-store.ts:60 — fireCount doc/semantics mismatch.
The doc-comment claimed "Number of mid-run TALON_FIRE: lines" but
the supervisor increments fireCount for every wake (mid-run and
terminal). Fixed the comment to match the actual semantics; same
counter, accurate meaning. Avoids breaking any caller that already
relies on fireCount as a total.
3. src/core/triggers.ts:fireWake — mid-run prompt says "Status: fired"
even when terminal=false, which can mislead downstream handling
into treating an in-flight watcher as a completed run. Decoupled
the prompt-display status from the on-disk TriggerStatus enum:
non-terminal fires now show "Status: signalled" in both the header
and the "Status:" line. Terminal fires unchanged.
4. src/core/triggers.ts:bufferAsPayload + fireWake — FIRE_PAYLOAD_MAX_BYTES
truncation used String.prototype.slice (UTF-16 code units), so a
payload of N multi-byte characters could exceed the documented byte
cap and split a character mid-codepoint. Introduced truncateUtf8Tail/
truncateUtf8Head helpers that encode to UTF-8 bytes, slice on a byte
boundary, then walk over UTF-8 continuation bytes (10xxxxxx) so we
never cut a multi-byte sequence in half. The byte cap is now actually
enforced as bytes.
5. src/storage/trigger-store.ts:282 + supervisor — updateTrigger only
marks the store dirty (10s autosave window). For terminal status
transitions (errored/cancelled/timed_out/fired) a crash inside that
window would leave on-disk status as "running", which loadTriggers()
then misclassifies as "terminated by previous restart". Added
persistNow() calls at the four supervisor transition sites:
failTrigger, cancelTrigger, the hard-timeout handler, and the
finalizeExit path. The on-disk store now always agrees with the
in-memory state across terminal transitions.
6. src/core/gateway-actions.ts:trigger_create — always returned
"Status: running" even when spawnTrigger() failed without throwing
(unsupported language slipping past validation, child.pid undefined,
failTrigger() routing). With #5 above, the store now reflects the
real state synchronously, so we re-read getTrigger(id) after spawn
and return ok:false + lastError when it landed in "errored", or
surface the actual status string otherwise. Callers no longer get
false success responses.
Regression tests (triggers-extended.test.ts):
- mid-run prompt asserts "Status: signalled" appears and "Status: fired"
does NOT, on a still-running trigger. Covers fix #3.
- multi-byte UTF-8 payload (2000× 💧 = 8000 bytes vs 4000 string length)
asserts the prompt's byte length stays bounded AND contains no
Unicode replacement character (U+FFFD), which would indicate a
mid-codepoint split. Covers fix #4.
The remaining Copilot thread (introducing a dedicated non-terminal
status enum value, replacing the TriggerStatus|"signalled" display-
status hack) is a wider type change with API/store implications;
better tackled in a follow-up than bundled into a defensive sweep.
Verification:
- npx vitest run → 1747/1748 pass (the 1 failure is the pre-existing
package.functional "Stopped" expectation that fails whenever a live
Talon daemon is running on the host — same env-dependent failure
noted in PR #144 and PR #90's verification blocks).
- npx vitest run src/__tests__/triggers* src/__tests__/trigger-store* →
74/74 pass (35 in triggers-extended including 2 new, 22 in
trigger-store, 17 in triggers).
- npx tsc --noEmit → clean.
- npx prettier --check on all 4 changed files → clean.
- npm run lint → 0 errors in changed files (10 pre-existing warnings
elsewhere unchanged).
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
---------
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-authored-by: claudiusthebot <claudius@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
CI StatusgateVerification
npm testnpm run test:functionalnpm run typechecknpm run lint(passes with existing warnings)npm run format:check(clean)npm run ci:protectNotes
main: strict required status checkCI Statusis enabled.format:checkis now strict — a Prettier failure fails the Code Quality job and blocks merge through theCI Statusgate.knip(dead code analysis) remains explicitly informational; the step name advertises that. Making it strict today would surface ~23 unused exports/types acrosssrc/backend/opencode/,src/core/, andsrc/frontend/terminal/that would need a separate cleanup.