feat(ci): on-demand tmux real-user testing for PRs - #5203
Merged
Conversation
yiliang114
force-pushed
the
feat/triage-tmux-testing
branch
2 times, most recently
from
June 16, 2026 15:26
47b07bc to
f47842b
Compare
Collaborator
Author
|
Validated end-to-end before review. Dispatched this workflow (dry-run, so no comment was left on the target PR) against a real TUI PR — #5003 ( Run: https://github.com/QwenLM/qwen-code/actions/runs/27628950209 — gating routed correctly (authorize/triage skipped, So the flow works. Verified on a real TUI PR rather than this one, since this PR is CI-only and has no TUI surface to drive. |
yiliang114
force-pushed
the
feat/triage-tmux-testing
branch
from
June 16, 2026 15:57
f47842b to
aeec2c0
Compare
yiliang114
marked this pull request as ready for review
June 16, 2026 16:09
Collaborator
|
Re-triage — reviewing latest push Template looks good ✓ On direction: unchanged — this fills a real gap. Static review and headless tests structurally can't catch TUI regressions; on-demand tmux testing as part of the triage pipeline is well-aligned with the project's needs. Part of #4866, clearly motivated. On approach: the scope grew from ~340 to ~680 lines across the review rounds, but the growth is security-driven and justified — container isolation, OpenAI proxy with path/method filtering, network proxy preservation, npm ci/build with proper error handling, and the HTML-escaping Moving on to code review. 🔍 中文说明重新 triage — 审查最新推送 模板完整 ✓ 方向：与之前一致——填补了真实的空白。静态审查和 headless 测试在结构上无法捕获 TUI 回归；作为 triage 流水线的一部分加入按需 tmux 测试，与项目需求高度对齐。属于 #4866 的一部分，动机清晰。 方案：范围从约 340 行增长到约 680 行，但增长由安全需求驱动且合理——容器隔离、带路径/方法过滤的 OpenAI 代理、网络代理保留、带正确错误处理的 npm ci/build、以及用 HTML 转义的 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Collaborator
|
Re-triage — code review of latest push Code ReviewCI-only change (one workflow file + actionlint config). Reviewing all findings from the prior four review rounds against the current code. All previous findings resolved:
One minor observation (non-blocking): Validation: TestingCI workflow change — no TUI surface to exercise. The workflow's own TUI-surface detection correctly classifies this PR as No tmux testing applicable for this PR. 中文说明代码审查纯 CI 变更（一个工作流文件 + actionlint 配置）。针对当前代码审查四轮审查的所有发现。 所有之前的发现已解决：
一个小观察（不阻塞）： 测试CI 工作流变更——无 TUI 界面可测试。工作流自身的 TUI 检测正确将此 PR 分类为 — Qwen Code · qwen3.7-max |
Collaborator
|
Stepping back to look at the whole picture after four rounds of review and iteration. Going back to my independent proposal: the PR's architecture matches what I'd design for on-demand real-user testing of PRs — trigger mechanism, permission gating, container isolation, credential isolation via proxy, result publishing. The author has been responsive to every finding across multiple rounds, and the code has genuinely improved: the security model went from "reasonable first pass" to "the strongest in the codebase for untrusted-code execution." The The only remaining observations are cosmetic: a dead The PR is CI-only (no TUI surface), so tmux testing is N/A — the workflow's own detection agrees. YAML validates, all 9 bash blocks pass Verdict: all findings from four review rounds are resolved. The security architecture is solid, the code is well-structured, and the author has been thorough in addressing feedback. Ready to ship. 中文说明经过四轮审查和迭代后退一步看全局。 回到我的独立方案：PR 的架构与我为按需真实用户测试设计的方案一致——触发机制、权限门控、容器隔离、通过代理的凭证隔离、结果发布。作者在多轮中对每个发现都做出了积极响应，代码真正得到了改进：安全模型从"合理的第一版"变成了"代码库中不可信代码执行最强的"。
剩余的观察都是表面的：一个无效的 PR 是纯 CI 的（无 TUI 界面），tmux 测试 N/A——工作流自身的检测也同意。YAML 验证通过，9 个 bash 块全部通过 结论： 四轮审查的所有发现已解决。安全架构扎实，代码结构良好，作者对反馈的处理很彻底。可以合并。 — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
approved these changes
Jun 16, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
qwen-code-ci-bot
added
scope/ci-cd
Contributor
There was a problem hiding this comment.
Pull request overview
Adds an on-demand “real-user” tmux testing path to the existing triage workflow, allowing maintainers to trigger an interactive TUI run against a PR’s merge ref and publish the outcome back to the PR while attempting to isolate write credentials from untrusted PR code execution.
Changes:
- Extends
qwen-triage.ymltriggers and authorization to support@qwen-code /tmuxand aworkflow_dispatchtmux_prinput. - Adds a self-hosted
tmux-testingjob that checks out the PR merge ref and runs the existingtmux-real-user-testingskill, uploading transcript/report artifacts. - Adds a separate GitHub-hosted
publish-tmuxjob to post/update a verdict comment on the PR.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
qwen-code-ci-bot
requested changes
Jun 16, 2026
Add a tmux-testing stage to qwen-triage.yml: a write-permission user runs `@qwen-code /tmux` on a PR (or dispatches with tmux_pr=N) to launch the changed app in a real tmux TUI session and exercise the affected flow, catching interactive regressions that diff-based review can't. - Gated on the PR AUTHOR having write permission (whose code runs), reusing the authorize job; no separate fork check. - Executes untrusted PR code with minimal blast radius: contents:read, no GitHub token in the agent env, persist-credentials:false. - Proxy bypass for the model call only. - A separate publish-tmux job (clean runner, write PAT, no PR-code checkout) posts the verdict back to the PR — keeping the write credential isolated from the code execution.
- Gate the workflow_dispatch tmux_pr path on the PR author's write permission via the authorize job, so dispatching never runs an unauthorized contributor's code on the self-hosted runner. - Make workflow_dispatch triage vs tmux mutually exclusive (tmux_pr set skips the triage job), matching the documented input contract. - Pin GITHUB_TOKEN/GH_TOKEN empty in the untrusted-code step so no inherited repo-scoped credential reaches the agent. - Serialize concurrent tmux runs per PR; skip PRs whose merge ref is unavailable (CONFLICTING); clean the runner workspace on exit. - publish-tmux now also reports infrastructure failures instead of staying silent, and the comment fence widens past any backtick run in untrusted output so it cannot break out and render markdown. - Correct the proxy-bypass comment to describe actual behavior.
yiliang114
force-pushed
the
feat/triage-tmux-testing
branch
from
June 17, 2026 07:56
195f00f to
8d3e271
Compare
Collaborator
Author
|
Pushed
On the proxy finding: I didn't copy the pr-review Verified end-to-end via a real |
- Fold should_test + has_tui into a single tri-state `decision` (skip | na | run); the five repeated step guards become `decision == 'run'`, and the standalone `Mark not applicable` step is gone (the n/a verdict is set inline in the resolve step). - Resolve PR state + file list in one `gh pr view` call instead of two. No behavior change: skip stays silent, na posts n/a, run drives the app.
Right after a /tmux comment GitHub may not have computed PR mergeability yet (mergeable=UNKNOWN), and refs/pull/N/merge is only current once it has. Retry the resolve up to 5x (3s apart) until it settles, so a freshly-pushed PR isn't checked out from a stale or missing merge ref.
Collaborator
Author
|
@qwen-code /triage |
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
Collaborator
Author
|
@qwen-code /triage |
qwen-code-ci-bot
requested changes
Jun 17, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
One remaining fix from the previous round: the emit_block function in publish-tmux still crashes when the report/transcript file contains no backtick characters. grep -oE returns exit code 1 on no match, and set -euo pipefail aborts the step before the comment is posted.
Fix — add || true after grep in the longest= line:
longest="$(printf '%s' "$content" | { grep -oE '`+' || true; } | awk '{ if (length > m) m = length } END { print m + 0 }')"Everything else in the re-triage looks solid — the security hardening, the rebase, the mutual exclusion. Just this one line. 🙏
qwen-code-ci-bot
requested changes
Jun 17, 2026
qwen-code-ci-bot
requested changes
Jun 17, 2026
- publish-tmux: render artifacts in HTML-escaped <pre> instead of a backtick fence; removes the grep crash on backtick-free content under set -euo pipefail and closes the </details>/fence-breakout injection - tmux-testing: skip when mergeability stays UNKNOWN after retries - classify exit 137/139 (OOM/segfault) as infra-error, not test fail - upload-artifact: continue-on-error so a pre-write crash isn't masked
qwen-code-ci-bot
requested changes
Jun 18, 2026
wenshao
requested changes
Jun 19, 2026
wenshao
reviewed
Jun 20, 2026
| # without honoring NO_PROXY. Clear proxy env for qwen itself while | ||
| # restoring it for child gh/git commands the agent may spawn. | ||
| # shellcheck disable=SC2016 | ||
| configure_qwen_network() { |
Collaborator
There was a problem hiding this comment.
[Suggestion] configure_qwen_network is a ~55-line function body that is near-identical to the one in .github/workflows/qwen-code-pr-review.yml. The only differences are the env var name parsed (REVIEW_OPENAI_BASE_URL vs OPENAI_BASE_URL), the error helper (echo "::error::" + exit 1 vs fail), and a couple of extra echo lines.
Consider extracting this into a shared composite action (e.g. .github/actions/configure-qwen-network/action.yml) or a sourced shell script to eliminate the duplication. This would make future changes to the network-stripping logic a single-file edit instead of two.
Collaborator
Author
There was a problem hiding this comment.
I am leaving this inline rather than extracting a shared action in this PR. The two call sites still differ in env names and trust boundary, and making a composite/shared helper would touch the existing review workflow as well. Worth doing as a follow-up once this path settles, but I do not want to expand this PR blast radius.
| # mergeability still UNKNOWN, or no TUI surface — stay silent. | ||
| if: >- | ||
| always() && github.event.inputs.skip_comment != 'true' && | ||
| (needs.tmux-testing.result == 'failure' || |
Collaborator
There was a problem hiding this comment.
[Suggestion] The three result values handled here are failure, cancelled, and success (with a non-empty, non-n/a verdict). The fourth possible value — skipped — is not handled.
When authorize denies permission for /tmux (the commenter or PR author lacks write access), tmux-testing is skipped (not failed), and publish-tmux's condition evaluates to false, so no feedback comment is posted. The requester gets a silent void that is indistinguishable from "it hasn't run yet."
Consider adding:
needs.tmux-testing.result == 'skipped' ||…with a corresponding branch in the script that posts a message like "/tmux was not executed because the PR author does not have write permission."
Collaborator
Author
There was a problem hiding this comment.
Leaving skipped out intentionally. A skipped tmux-testing job is the normal no-op path for non-/tmux, unauthorized, or otherwise non-runnable triggers; publishing on skipped would turn unrelated comments/events into PR noise. The explicit surfaced outcomes are failure, cancelled, and successful runs with a real verdict.
yiliang114
force-pushed
the
feat/triage-tmux-testing
branch
from
June 21, 2026 03:09
fc716ad to
7dc28f8
Compare
wenshao
requested changes
Jun 21, 2026
Collaborator
Author
|
@qwen-code /triage |
qwen-code-ci-bot
approved these changes
Jun 21, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
wenshao
approved these changes
Jun 21, 2026
wenshao
requested changes
Jun 21, 2026
Comment on lines
+781
to
+783
| content="${content//&/&}" | ||
| content="${content//</<}" | ||
| content="${content//>/>}" |
Collaborator
There was a problem hiding this comment.
[Critical] The HTML escaping here is silently defeated on bash ≥ 5.2. In ${var//pat/repl}, bash 5.2+ treats an unescaped & in the replacement as a back-reference to the matched text. So ${content//</<} expands to <lt; (the & becomes the matched <), not < — < and > are never escaped. Only the first line works, by luck (its match is &, so & happens to be right).
Verified on bash 5.2.37 (the version ubuntu-latest, where publish-tmux runs, ships): input a<b>&c → a<lt;b>gt;&c, not the intended a<b>&c.
Impact: the breakout this function was rewritten to prevent is still open. Untrusted PR output (report.md / transcript) containing e.g. </code></pre></details><img src=x onerror=...> escapes the <pre><code> / <details> block and renders as live HTML in a comment posted with the write-scoped CI_BOT_PAT. Legitimate </> in normal logs are also corrupted to <lt;/>gt;.
Escape the & in each replacement so it is treated literally (order stays &, then <, then >):
Suggested change
| content="${content//&/&}" | |
| content="${content//</<}" | |
| content="${content//>/>}" | |
| content="${content//&/\&}" | |
| content="${content//</\<}" | |
| content="${content//>/\>}" |
— claude-opus-4-8[1m] via Qwen Code /qreview
| verdict: '${{ steps.run.outputs.verdict || steps.prepare.outputs.verdict || steps.pr.outputs.verdict }}' | ||
| failure_phase: '${{ steps.prepare.outputs.failure_phase }}' | ||
| steps: | ||
| - name: 'Install tmux runner tools' |
Collaborator
There was a problem hiding this comment.
[Suggestion] This step has no if: guard, so the full apt-get install (curl/git/gnupg/jq/tmux + the gh keyring dance) and npm install -g @qwen-code/qwen-code@latest run on every invocation — before the next step (Resolve PR and check state) computes decision. Every later step is gated on decision == 'run', but by then the toolchain is already installed.
For the common decision=na case (PR touches no TUI surface) and all skip cases (closed/draft/conflicting/UNKNOWN), the shared self-hosted ecs-qwen runner pays the entire setup cost for a job that then does nothing. Because runs are serialized (cancel-in-progress: false), that wasted setup also holds the runner and delays the next queued real test.
Consider installing only what Resolve PR and check state needs (jq, gh) up front, running the state check first, and gating the heavy apt-get + global-npm block on steps.pr.outputs.decision == 'run'.
— claude-opus-4-8[1m] via Qwen Code /qreview
| { | ||
| printf '%s\n\n' '<!-- qwen-triage:tmux -->' | ||
| printf '**tmux real-user testing: %s** - [workflow run](%s)\n\n' "$VERDICT" "$RUN_URL" | ||
| printf 'Launched the changed app in a real tmux session and exercised the affected flow.\n\n' |
Collaborator
There was a problem hiding this comment.
[Suggestion] The Run step deliberately distinguishes timeout (exit 124) and infra-error (SIGKILL 137 / SIGSEGV 139 — OOM, crash, forced timeout) from a genuine fail, with a comment that these are "not a test outcome." But both still produce result=success + a non-empty, non-n/a verdict, so they pass every branch above and land in this final else — which prints "Launched the changed app in a real tmux session and exercised the affected flow" plus the (for an OOM/timeout, typically empty or truncated) report and transcript blocks.
So a run that crashed/OOM'd/timed out before exercising anything is reported with a body asserting the flow was exercised — reintroducing at the rendering layer the exact "verdict misleads" outcome the infra-error classification was added to prevent. Consider an explicit branch for timeout/infra-error before this else whose body doesn't claim the flow ran (e.g. "the app launched but testing did not complete — it timed out or the process was killed; this is not a pass/fail result").
— claude-opus-4-8[1m] via Qwen Code /qreview
| "GITHUB_TOKEN=" | ||
| "GH_TOKEN=" | ||
| "OPENAI_API_KEY=qwen-loopback-proxy" | ||
| "OPENAI_BASE_URL=$LOCAL_OPENAI_BASE_URL" |
Collaborator
There was a problem hiding this comment.
[Suggestion] QWEN_ENV (handed to the agent via env -i) sets OPENAI_BASE_URL and OPENAI_API_KEY but not OPENAI_MODEL. The outer agent gets its model from --model "$OPENAI_MODEL" (QWEN_CMD), but the tmux-real-user-testing skill launches the app under test with npm run dev -- --approval-mode yolo and no --model, so it reads the model from the environment — which here lacks OPENAI_MODEL and falls back to the built-in default (coder-model).
The loopback proxy forwards that request upstream where only vars.QWEN_PR_REVIEW_MODEL is provisioned; if the upstream doesn't serve coder-model, the app-under-test's model calls fail and the "real-user" session can't actually exercise anything (while the outer agent may still exit 0 → a pass verdict). Add "OPENAI_MODEL=${OPENAI_MODEL:-}" to QWEN_ENV so the app under test runs on the same model as the harness.
— claude-opus-4-8[1m] via Qwen Code /qreview
| local proxy_port proxy_script | ||
| proxy_port=8787 | ||
| proxy_script="${RUNNER_TEMP:-/tmp}/qwen-openai-proxy.js" | ||
| cat > "$proxy_script" <<'NODE' |
Collaborator
There was a problem hiding this comment.
[Suggestion] The OpenAI proxy on port 8787 has no rate limiting, request count cap, or body size limit. The untrusted node user (running PR code with --approval-mode yolo) can send unlimited POST /chat/completions requests for the full 20-minute window. Each request is forwarded to the upstream API with the real REVIEW_OPENAI_API_KEY, so a malicious or compromised PR could cause significant API billing damage (thousands of parallel requests with expensive models and large context windows).
Suggested change
| cat > "$proxy_script" <<'NODE' | |
| const MAX_REQUESTS = 200; | |
| const MAX_BODY = 4 * 1024 * 1024; | |
| let requestCount = 0; | |
| const server = http.createServer(async (req, res) => { | |
| if (++requestCount > MAX_REQUESTS) { | |
| res.writeHead(429, { 'content-type': 'text/plain' }); | |
| res.end('proxy: request limit exceeded\n'); | |
| return; | |
| } |
— qwen3.7-max via Qwen Code /review
| return; | ||
| } | ||
|
|
||
| const headers = new Headers(req.headers); |
Collaborator
There was a problem hiding this comment.
[Suggestion] The proxy forwards all incoming headers except host and content-length to the upstream API. The untrusted node user can inject headers like OpenAI-Organization or OpenAI-Project to redirect billing to a different organization that the API key has access to, or inject X-Forwarded-For to bypass IP-based rate limiting.
Replace the denylist approach with an allowlist of safe headers:
Suggested change
| const headers = new Headers(req.headers); | |
| const headers = new Headers(); | |
| const contentType = req.headers['content-type']; | |
| if (contentType) headers.set('content-type', contentType); | |
| const accept = req.headers['accept']; | |
| if (accept) headers.set('accept', accept); | |
| headers.set('authorization', `Bearer ${apiKey}`); |
— qwen3.7-max via Qwen Code /review
| } else { | ||
| res.end(); | ||
| } | ||
| } catch (error) { |
Collaborator
There was a problem hiding this comment.
[Suggestion] The error handler returns the raw error.message from Node.js fetch(), which can include the upstream hostname and IP in connection errors (e.g., ECONNREFUSED 10.0.1.5:443 or ENOTFOUND internal-llm-proxy.corp). This leaks internal API infrastructure details to the untrusted node user.
Suggested change
| } catch (error) { | |
| res.end('proxy error: upstream request failed\n'); |
Log the detailed error to stderr instead (visible in workflow logs but not to the agent):
console.error(`proxy upstream error: ${error instanceof Error ? error.message : String(error)}`);— qwen3.7-max via Qwen Code /review
| "GH_TOKEN=" | ||
| "OPENAI_API_KEY=qwen-loopback-proxy" | ||
| "OPENAI_BASE_URL=$LOCAL_OPENAI_BASE_URL" | ||
| "NO_PROXY=${NO_PROXY:-}" |
Collaborator
There was a problem hiding this comment.
[Suggestion] NO_PROXY and no_proxy are passed to the node user via QWEN_ENV. These contain the internal API hostname (appended by configure_qwen_network), which leaks the internal API endpoint to the untrusted PR code. The node user can read its own environment to discover the hostname.
The qwen process only talks to 127.0.0.1:8787 (loopback), which does not need NO_PROXY. Configure NO_PROXY only in the wrapper scripts for gh/git child processes, not in the QWEN_ENV array:
Suggested change
| "NO_PROXY=${NO_PROXY:-}" | |
| "NO_PROXY=" | |
| "no_proxy=" |
— qwen3.7-max via Qwen Code /review
|
|
||
| if [ "$EXIT_CODE" -eq 124 ]; then | ||
| VERDICT='timeout' | ||
| elif [ "$EXIT_CODE" -eq 137 ] || [ "$EXIT_CODE" -eq 139 ]; then |
Collaborator
There was a problem hiding this comment.
[Suggestion] Exit 137 can come from two distinct sources: (1) timeout --kill-after=10s 20m sending SIGKILL when the process ignores SIGTERM (a test timeout), or (2) a genuine OOM kill / segfault. Both are classified as infra-error, but case 1 is actually a test timeout. The PR comment for infra-error says "checkout, runner, or setup error" which is misleading for a test that simply ran too long.
Check the elapsed time to distinguish the two cases:
Suggested change
| elif [ "$EXIT_CODE" -eq 137 ] || [ "$EXIT_CODE" -eq 139 ]; then | |
| RUN_START=$(date +%s) | |
| set +e | |
| timeout --kill-after=10s 20m runuser -u node -- env -i "${QWEN_ENV[@]}" "${QWEN_CMD[@]}" \ | |
| --prompt "/tmux-real-user-testing ${PR_NUMBER} --repo ${REPOSITORY}" \ | |
| --output-format stream-json \ | |
| | tee "$RUNNER_TEMP/tmux-results/output.jsonl" | |
| EXIT_CODE=${PIPESTATUS[0]} | |
| RUN_ELAPSED=$(( $(date +%s) - RUN_START )) | |
| set -e |
Then in the verdict mapping:
elif [ "$EXIT_CODE" -eq 137 ] || [ "$EXIT_CODE" -eq 139 ]; then
if [ "$RUN_ELAPSED" -ge 1190 ]; then
VERDICT='timeout'
else
VERDICT='infra-error'
fi— qwen3.7-max via Qwen Code /review
| PREPARE_FAILURE_PHASE: '${{ needs.tmux-testing.outputs.failure_phase }}' | ||
| TMUX_RESULT: '${{ needs.tmux-testing.result }}' | ||
| RUN_URL: '${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}' | ||
| # shellcheck disable=SC2016 |
Collaborator
There was a problem hiding this comment.
[Suggestion] The # shellcheck disable=SC2016 comment is placed at the YAML indentation level (8 spaces, sibling of run:), making it a YAML comment invisible to shellcheck. Compare with line 438 in the same PR where the same directive is correctly placed inside the run: |- block at 10 spaces indentation.
Move the directive inside the run block:
Suggested change
| # shellcheck disable=SC2016 | |
| run: |- | |
| # shellcheck disable=SC2016 | |
| set -euo pipefail |
— qwen3.7-max via Qwen Code /review
| # package scripts/builds cannot persist changes in the self-hosted runner's | ||
| # host filesystem across workflow runs. | ||
| container: | ||
| image: 'node:22-bookworm' |
Collaborator
There was a problem hiding this comment.
[Suggestion] The container image node:22-bookworm is referenced by mutable tag only, while every GitHub Action in this file is pinned by SHA digest (actions/checkout@de0fac2e..., etc.). This is the only unpinned external dependency in the workflow. A tag-only reference means a substituted or compromised image on Docker Hub would be silently pulled, bypassing the supply-chain hardening applied to all other dependencies.
Pin to a digest:
Suggested change
| image: 'node:22-bookworm' | |
| image: 'node:22-bookworm@sha256:<digest>' # node:22-bookworm |
— qwen3.7-max via Qwen Code /review
| # Post the tmux verdict back to the PR. Runs on a clean GitHub-hosted runner | ||
| # with the write PAT and never checks out PR code, so the write credential is | ||
| # isolated from the untrusted-code execution in tmux-testing above. | ||
| publish-tmux: |
Collaborator
There was a problem hiding this comment.
[Suggestion] The publish-tmux job has no timeout-minutes setting, while every other job in this workflow sets an explicit timeout (authorize: 5, triage: 30, tmux-testing: 45). This job only downloads an artifact and posts a PR comment — work that should complete in under a minute. Without a timeout, a hung gh api call could occupy a runner slot for up to GitHub's 360-minute default.
Suggested change
| publish-tmux: | |
| publish-tmux: | |
| needs: ['tmux-testing'] | |
| timeout-minutes: 5 |
— qwen3.7-max via Qwen Code /review
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
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.
What this PR does
Adds an on-demand real-user testing stage to the triage workflow. A write-permission user runs
@qwen-code /tmuxon a PR (or dispatches the workflow withtmux_pr=<N>); a self-hosted job checks out the PR's merge ref, launches the changed app in a real tmux TUI session via the existingtmux-real-user-testingskill, exercises the affected flow, and a separate job posts the verdict back to the PR.Why it's needed
For a TUI app, the regressions most likely to slip through are interactive ones — rendering, dialogs, keyboard navigation, slash commands, auth/model flows. The existing
/reviewreasons over the diff statically and cannot see whether the app still works when a user runs it; headless tests cover tool/API behavior, not the interactive journey. This stage is the only one that actually runs the changed code as a user, so it catches what the others structurally can't — essentially automating the maintainer's "pull it and try it," with a readable transcript attached.Reviewer Test Plan
How to verify
CI workflow change. Verified locally:
actionlintreports only the two pre-existing-style findings (theecs-qwencustom self-hosted label, same as the review workflow; and an info-level SC2016 on an intentionally single-quoted jq filter), YAML parses, andbash -npasses on every new run script. Behaviorally validated by dispatching the branch workflow against a real PR (workflow_dispatchwithtmux_pr) — the gating routed correctly (authorize/triage skipped,tmux-testingran on the self-hosted runner).Evidence (Before & After)
N/A (non–user-visible CI change). The stage itself produces the evidence: a per-run transcript artifact plus a verdict comment on the PR.
Tested on
Risk & Scope
authorizejob, not a separate fork check; runs withcontents: read, no GitHub token in the agent env, andpersist-credentials: false. The proxy is bypassed for the model call only.publish-tmuxjob on a clean GitHub-hosted runner that never checks out PR code./reviewinstead of on-demand.authorizegate); base will retarget tomainonce fix(ci): gate PR review and triage on write permission #5191 merges.Linked Issues
Part of splitting #4866 (the triage pipeline). No closing keyword.
中文说明
这个 PR 做了什么
给 triage 工作流加一个按需的真实用户测试阶段。有 write 权限的人在 PR 上评论
@qwen-code /tmux(或用tmux_pr=<N>dispatch 工作流);一个自托管 job 会 checkout PR 的 merge ref、用现成的tmux-real-user-testingskill 在真实 tmux TUI 里把改动后的 app 跑起来、走一遍受影响的流程,再由一个单独的 job 把结论发回 PR。为什么需要
对 TUI 应用,最容易漏的回归是交互类的——渲染、对话框、键盘导航、slash 命令、auth/model 流。现有的
/review是对 diff 做静态推理,看不到"app 跑起来还能不能用";headless 测试覆盖的是工具/API 行为,不是交互旅程。这个阶段是唯一真正"把改动后的代码当用户跑一遍"的,能抓住前两者结构上抓不到的东西——本质上把维护者"拉下来试一下"自动化了,并附上可读的 transcript。风险与范围
authorizejob,而非单独的 fork 检查;以contents: read运行、agent 环境里没有 GitHub token、persist-credentials: false。代理只对模型调用绕过。publish-tmuxjob 在干净的 GitHub 托管 runner 上发布,它从不 checkout PR 代码。/review之后自动跑而非按需。authorize门);fix(ci): gate PR review and triage on write permission #5191 合并后 base 自动 retarget 到main。关联 Issue
#4866(triage 流水线)拆分的一部分。不带关闭关键字。