Skip to content

fix: keep bounded event consume runs alive after stdin EOF#1285

Open
Emrys1105 wants to merge 6 commits into
mainfrom
fix/event-consume-stdin-eof
Open

fix: keep bounded event consume runs alive after stdin EOF#1285
Emrys1105 wants to merge 6 commits into
mainfrom
fix/event-consume-stdin-eof

Conversation

@Emrys1105
Copy link
Copy Markdown
Collaborator

@Emrys1105 Emrys1105 commented Jun 5, 2026

Summary

event consume armed its stdin-EOF shutdown watcher for every non-TTY session, so subprocess callers whose stdin is /dev/null (OpenCode, CI, bash -c) were cancelled at startup with Error: context canceled — even when --max-events/--timeout explicitly declared the run's lifecycle (#1131). This PR gates the watcher to non-TTY unbounded runs only, so bounded runs exit via their own bound, timeout, or SIGTERM. Supersedes #1183 (cherry-picked with author attribution preserved) and adds the documentation/contract sync it lacked.

Changes

  • Gate the stdin-EOF watcher with shouldWatchStdinEOF (non-TTY && maxEvents <= 0 && timeout <= 0) in cmd/event/consume.go — cherry-picked from fix(event): keep bounded consume alive after stdin EOF #1183 (author @jinhyuk9714)
  • Extend TestShouldWatchStdinEOF with combined / mixed-sign bound cases in cmd/event/consume_stdin_test.go (9 cases total)
  • Drop the now-misleading "or close stdin" stop hint for bounded runs in internal/event/consume/consume.go, with tests in internal/event/consume/listening_text_test.go
  • Sync skills/lark-event/SKILL.md: bounded-run stdin-EOF exemption, exit-reason table note, and description routing (static event schema lookup, --as bot identity)
  • Append "Bounded runs ignore stdin EOF." to --max-events / --timeout flag help in cmd/event/consume.go

Behavior change: a bounded run no longer stops early when a piped stdin closes — it exits only via its bound, timeout, or SIGTERM. Residual (accepted): unbounded runs over /dev/null keep the documented legacy instant graceful exit (reason: signal); under a cold-start race this can surface as exit 1 context canceled (pre-existing, out of scope).

Test Plan

  • make unit-test passed (full unit + integration suite, via harness validate)
  • validate passed (build / vet / unit / integration / convention / security)
  • local-eval passed (E2E 2/2 passed + 1 fixture-gated skip backed by live manual evidence; skillave 4/4)
  • acceptance-reviewer passed (7/7 cases)
  • manual verification: lark-cli event consume im.message.receive_v1 --as bot --max-events 1 --timeout 10m < /dev/null in the sandbox container — stayed alive 5m07s on /dev/null stdin, received 1 real IM event, exited 0 with reason: limit

Related Issues

Closes #1131

Supersedes #1183 (commits cherry-picked with original authorship preserved).

Summary by CodeRabbit

  • Documentation

    • Enhanced --max-events help text to clarify how stdin-EOF shutdown behaves for bounded runs.
    • Updated skill documentation with clearer guidance on stdin EOF behavior and its constraints.
  • Improvements

    • Refined shutdown mechanism to conditionally enable stdin-EOF watching only for unbounded runs.
    • Updated stop instructions for more appropriate guidance based on whether runs are bounded or unbounded.

@Emrys1105 Emrys1105 added bug Something isn't working domain/event Event subscription domain labels Jun 5, 2026
@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Jun 5, 2026

CLA assistant check
All committers have signed the CLA.

@github-actions github-actions Bot added size/L Large or sensitive change across domains or core paths and removed domain/event Event subscription domain labels Jun 5, 2026
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Jun 5, 2026

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: d114a4c2-d29b-4664-b572-de8608258fc3

📥 Commits

Reviewing files that changed from the base of the PR and between 62364fc and 57ee10e.

📒 Files selected for processing (5)
  • cmd/event/consume.go
  • cmd/event/consume_stdin_test.go
  • internal/event/consume/consume.go
  • internal/event/consume/listening_text_test.go
  • skills/lark-event/SKILL.md

📝 Walkthrough

Walkthrough

This PR refines stdin EOF shutdown behavior for the event consume command. A new shouldWatchStdinEOF predicate gates stdin-EOF watching to non-TTY unbounded runs only. Bounded runs (with --max-events or --timeout) now display guidance that omits "close stdin" instructions. Documentation is updated to clarify this conditional behavior.

Changes

Conditional stdin EOF watching for bounded event consumption runs

Layer / File(s) Summary
stdin EOF watching predicate and CLI wiring
cmd/event/consume.go, cmd/event/consume_stdin_test.go
shouldWatchStdinEOF(isTerminal, maxEvents, timeout) centralizes the logic that enables stdin-EOF shutdown only for non-TTY unbounded runs. The consume command wiring uses this predicate instead of a simple non-TTY check. The --max-events flag help text is extended to document that bounded runs ignore stdin-EOF shutdown.
Bounded run stop guidance updates
internal/event/consume/consume.go, internal/event/consume/listening_text_test.go
stopHintText now accepts Options and returns context-aware guidance: bounded runs instruct SIGTERM kill only, while unbounded runs include both SIGTERM and stdin-close instructions. Tests distinguish unbounded (includes stdin closure) from bounded (no stdin closure) behavior.
Documentation clarifications
skills/lark-event/SKILL.md
Top-level description now includes --as bot flag usage. Stdin EOF behavior clarifies that EOF is ignored for bounded runs. Exit code table specifies stdin EOF applies only to unbounded runs.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Poem

🐰 A rabbit hops through bounded time,
No stdin closing—quite sublime!
For unbounded hopes and dreams so free,
Close stdin or SIGTERM, take your key!
Thump thump goes the predicate gate,
Deciding when to shut down—no wait!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 58.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: fixing bounded event consume runs to stay alive after stdin EOF, which is the core behavioral change across multiple files.
Description check ✅ Passed The PR description comprehensively covers motivation, specific code changes, testing (unit/integration/manual), and closes issue #1131. All required template sections are completed.
Linked Issues check ✅ Passed The PR directly addresses issue #1131 by gating stdin-EOF watching to non-TTY unbounded runs only, preventing premature shutdown when --max-events or --timeout are provided, which was the core requirement.
Out of Scope Changes check ✅ Passed All changes are tightly scoped to the stdin-EOF handling fix: the gating predicate, tests, stop-hint adjustments, documentation updates, and flag help text. No extraneous modifications detected.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/event-consume-stdin-eof

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jun 5, 2026

🚀 PR Preview Install Guide

🧰 CLI update

npm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@57ee10e92382ed085348c15cf362685446450b46

🧩 Skill update

npx skills add larksuite/cli#fix/event-consume-stdin-eof -y -g

@codecov
Copy link
Copy Markdown

codecov Bot commented Jun 5, 2026

Codecov Report

❌ Patch coverage is 80.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.17%. Comparing base (639259f) to head (57ee10e).
⚠️ Report is 42 commits behind head on main.

Files with missing lines Patch % Lines
cmd/event/consume.go 80.00% 1 Missing ⚠️
internal/event/consume/consume.go 80.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1285      +/-   ##
==========================================
+ Coverage   69.17%   70.17%   +1.00%     
==========================================
  Files         630      671      +41     
  Lines       59380    65334    +5954     
==========================================
+ Hits        41075    45850    +4775     
- Misses      14996    15795     +799     
- Partials     3309     3689     +380     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working size/L Large or sensitive change across domains or core paths

Projects

None yet

Development

Successfully merging this pull request may close these issues.

opencode 中运行lark-event技能, 监听一个条消息, 然后自动回复

3 participants