ci(issue-welcome): apply needs-triage label instead of welcome comment#73
Conversation
Replace the hardcoded bilingual bug/feature welcome copy with a single deterministic action: apply a `needs-triage` label (auto-created if missing). Greeting, content classification, and triage commentary are owned by the dedicated triage agent (octo-issue-notify webhook mode), so the in-workflow copy matrix was redundant. Bot issues skipped; idempotent; least-privilege unchanged (issues: write). Filename/interface unchanged — no caller migration needed.
|
齐静春 📖 review — verdict: APPROVED
In-place rewrite of the Verified against head 🟢 Caller-migration-free claim is correct. Diff shows 🟢 Separation-of-concerns is the right axis. Greeting + classification + triage commentary are LLM-suited and live in the dedicated triage agent (octo-issue-notify webhook mode). Workflow keeps only the deterministic, always-on, no-LLM part. This kills the duplication the old code had — the hardcoded bug/feature copy matrix and the triage agent's smart classification were two answers to "what is this issue". Now there is one answer. 🟢 Permissions stay least-privilege. Workflow-level 🟢 Label-not-found + race handling is correct.
🟢 Bot-skip guard is hardened. 🟡 Non-blocking nit: LGTM — ship it. — 齐静春 📖 |
mochashanyao
left a comment
mochashanyao
left a comment
There was a problem hiding this comment.
[Octo-Q · automated review]
Verdict: Approve — no blocking findings; notes below (data-flow traced).
Review Report: .github PR#73 — ci(issue-welcome): apply needs-triage label instead of welcome comment
Reviewer: Octo-Q (automated review)
PR: #73
Head SHA: 6739f88
Scope: 1 file changed (+39 / −103), .github/workflows/issue-welcome.yml
1. Verification Summary
| Item | Status | Evidence |
|---|---|---|
workflow_call interface unchanged |
✅ | No inputs/secrets added or removed; on: workflow_call: identical |
| Caller compatibility (19 repos) | ✅ | All callers use uses: …/issue-welcome.yml@v1 with no inputs; internal job rename welcome→label is invisible to callers |
| Permissions least-privilege | ✅ | permissions: {} at top level, issues: write at job level — unchanged from base |
| Bot guard preserved | ✅ | issue.user && issue.user.type === 'Bot' check retained (line 40–42 of new file) |
| Ghost-user guard preserved | ✅ | Comment + null-check on issue.user retained |
| Idempotency | ✅ | addLabels is idempotent (GitHub de-dupes); label auto-created if missing with race-condition tolerance |
| Concurrency group | ✅ | Renamed welcome→needs-triage with same ${{ github.event.issue.number }} key; cancel-in-progress: false unchanged |
actions/github-script pinned SHA |
✅ | 3a2844b7e9c422d3c10d287c895573f7108da1b3 (v9.0.0) — unchanged from base |
| Error handling correctness | ✅ | getLabel 404 → create; non-404 re-thrown; createLabel swallows races; addLabels failure propagates and fails the step |
2. Findings
No P0 or P1 findings.
P2 — createLabel error swallowing is broader than documented
Location: Lines 55–58 of the new file (inner catch (e) block).
} catch (e) {
// Another concurrent run may have created it; ignore races.
core.debug(`createLabel race/err: ${e.message}`);
}Diff-scope: New code introduced by this PR.
Analysis: The comment says "ignore races" but the catch swallows all errors from createLabel, not just 409/race conditions. If createLabel fails for a non-race reason (e.g., token lacks repo:scope in a future permission change, or the repo has label creation disabled), the error is silently logged at debug level and execution continues to addLabels, which will then fail with a 422 "Label does not exist" error. The net behavior is still correct (the step fails), but the root cause in logs will point to addLabels rather than the actual createLabel failure.
Severity: P2 (maintainability / debuggability). No user-visible incorrect behavior — the workflow step still fails, just with a less precise error message.
Suggestion: Narrow the inner catch to also check e.status === 409 (or at least log at core.warning for non-409 statuses) so that unexpected failures surface clearly:
} catch (e) {
if (e.status === 409) {
core.debug(`createLabel race (concurrent run): ${e.message}`);
} else {
core.warning(`createLabel unexpected error: ${e.status} ${e.message}`);
}
}P2 — Orphaned marker comments on existing issues
Diff-scope: Pre-existing data; PR removes the code that writes the marker but does not address cleanup.
Analysis: The old workflow embedded <!-- octo-issue-welcome:v1 --> in welcome comments on previously-triaged issues. After this PR merges, those markers become orphaned HTML in historical comments — invisible to users (rendered as HTML comments) and harmless. No action required, but noting for completeness.
Severity: P2 (informational). No functional impact.
3. Data Flow Trace
| Consumed Data | Source | Flows Correctly? |
|---|---|---|
context.payload.issue |
GitHub Actions event context (issues: opened) |
✅ — guard at top with core.setFailed if undefined |
issue.user.type |
GitHub API issue object | ✅ — null-guarded with issue.user && |
context.repo.owner / context.repo.repo |
GitHub Actions context | ✅ — standard, unchanged from base |
issue.number |
GitHub API issue object | ✅ — used in concurrency group and addLabels call |
LABEL constant ('needs-triage') |
Hardcoded in script | ✅ — deterministic, no upstream dependency |
getLabel response |
GitHub REST API | ✅ — only used for existence check; 404 triggers create path |
createLabel result |
GitHub REST API | ✅ — result not consumed; side-effect only; errors handled |
addLabels result |
GitHub REST API | ✅ — terminal call; failure propagates to step status |
4. Security Review
- Token scope:
issues: writeis the minimum required for label operations. ✅ - No secret exfiltration: No outbound HTTP calls, no data egress. ✅
- Supply chain:
actions/github-scriptpinned to full SHA (3a2844b…). ✅ - No user-controlled input in shell commands: All logic runs in github-script sandbox. ✅
GITHUB_TOKENauto-injected: Standard Actions token, scoped to the calling repo. ✅
5. Additional Observations
- v1 tag update: After merge, the
v1tag must be moved to the new commit for callers to pick up the change. This is standard operational procedure and not a PR concern, but worth tracking. - Triage agent dependency: The PR description states the triage agent (octo-issue-notify webhook mode) owns greeting + classification + label removal. This creates an implicit contract: if the triage agent is down or
TRIAGE_WEBHOOK_URLis not configured for a repo, issues will accumulate theneeds-triagelabel without being triaged. This is a deployment/ops concern, not a code concern.
[Octo-Q] verdict: APPROVE
Rationale: No P0/P1 findings. The PR is a clean, well-documented simplification that correctly reduces the workflow to a deterministic label application while preserving all safety guards (bot skip, ghost user, idempotency, concurrency, least-privilege). The workflow_call interface is unchanged, so all 19 caller repos require zero migration. Two P2 findings (error swallowing breadth, orphaned markers) are non-blocking.
yujiawei
left a comment
yujiawei
left a comment
There was a problem hiding this comment.
Code Review — PR #73 (.github)
Summary
This replaces the hardcoded bilingual welcome-comment matrix in the reusable issue-welcome.yml with a single deterministic action: apply a needs-triage label (auto-created if missing). The greeting/classification responsibility moves to the dedicated triage agent. Net +39 / -103, one file. The change is correct, the public-interface contract is preserved, and the security posture is actually improved over the prior version.
1. Verification
- ✅
workflow_callinterface unchanged —on: workflow_callwith no inputs/secrets, andpermissions: { issues: write }are identical to the prior version. Verified a representative caller (Mininglamp-OSS/octo-server/.github/workflows/issue-welcome.yml) passes no inputs and only grantsissues: writeonissues: [opened]. No caller migration needed, as claimed. (15+ caller repos resolved to the same shape via org-wide search.) - ✅ Idempotency — relies on GitHub de-duping
addLabelsfor the same label name; re-runs are harmless. This correctly replaces the oldlistComments+ marker scan.addLabelswith an existing label is a no-op. - ✅ Concurrency-race handling on label creation —
getLabel→ on404→createLabel, with thecreateLabelerror swallowed viacore.debugso a concurrent run that already created the label does not fail this run. Sound. - ✅ Bot / ghost-user guard —
issue.user && issue.user.type === 'Bot'correctly skips bot-opened issues while guarding against a nullissue.user(deleted accounts). - ✅ Least privilege — top-level
permissions: {}, job-levelissues: writeonly;GITHUB_TOKENpassed explicitly.createLabel+addLabelsboth fall underissues: write. No scope creep. - ✅
actionlint+ workflow-sanity (no-tabs) checks — both green on head SHA.
2. Security review (PR classified security-sensitive)
- ✅ No script-injection surface. All values consumed inside the
actions/github-scriptblock come from the runtimecontext.payload/context.repoobjects, not from${{ ... }}template interpolation into the script body. There is no path where untrusted issue title/body is expanded into the inline script. Notably, the new version removes the old language-detection logic that readissue.title/issue.body, so the workflow no longer touches attacker-controlled issue content at all — strictly reduced attack surface. - ✅ No secret egress. The only secret used is
GITHUB_TOKEN; nothing is forwarded to an external endpoint here. - ✅ Pinned action.
actions/github-scriptpinned to a full commit SHA (3a2844b…, v9.0.0). Good supply-chain hygiene.
Nothing here requires manual human verification beyond the product-coupling note below.
3. Findings
No P0/P1 issues. The change is correct and safe to merge.
P2 — Org-wide acknowledgment now depends on the triage agent being wired up in each caller repo (worth a human decision)
This is the one item I'd want a maintainer to confirm rather than a code defect. The PR description states the triage agent now "owns greeting, classification, and removing the label." But issue-welcome.yml and octo-issue-notify.yml are independent reusables that callers opt into separately: a caller can use issue-welcome.yml@v1 without also wiring octo-issue-notify.yml + the optional TRIAGE_WEBHOOK_URL secret. In any such repo, a newly-opened community issue will now receive a needs-triage label and zero human-visible acknowledgment (the previous bilingual welcome comment is gone). For a public OSS org this is a contributor-facing behavior change. Recommendation: confirm every repo that consumes issue-welcome.yml also has the triage webhook configured, or accept the "label-only, no comment" outcome as intended. Not a merge blocker — the label itself applies correctly.
Nits (non-blocking, no change required)
- The
getLabel→createLabeldance is partially redundant:addLabelsauto-creates a missing label on the fly. The explicit create is still justified here because it pins a deterministic color (d876e3) and description (Awaiting triage) instead of GitHub's default — so this is a reasonable, intentional choice, not dead code. - A
createLabelfailure for a non-race reason (e.g. validation/permission) is also swallowed by thecatch, after whichaddLabelswould surface the underlying failure (and auto-create with a default color). Acceptable — the job still fails loudly on a genuine permission problem — but if you ever want the create error to be attributable, distinguishing race (422 already_exists) from other failures would make logs clearer.
4. Verdict
Correct, well-documented, security posture improved, public interface preserved. Approving. The P2 is a product/ops confirmation for maintainers, not a code blocker.
Jerry-Xin
left a comment
Jerry-Xin
left a comment
There was a problem hiding this comment.
This PR is relevant to Mininglamp-OSS/.github, but it changes the ownership contract for issue greeting/triage without updating the repository’s architecture baseline.
🔴 Blocking
- 🔴 Critical —
.github/workflows/issue-welcome.yml:7now says the triage agent owns greeting, whiledocs/workflow-architecture.md:284-296explicitly says the agent does not own greeting, anddocs/workflow-architecture.md:300-303saysissue-welcomemust remain the native, reliable first-response workflow. That document is the repo’s stated automation acceptance baseline, so merging this as-is leaves the implementation and design contract in direct conflict. Either update the architecture/agent contract in the same PR, or keep the native greeting behavior.
💬 Non-blocking
- 🟡 Warning —
.github/workflows/issue-welcome.yml:62-65catches allcreateLabelfailures and labels them as a race. Consider only suppressing the expected “already exists” race, and rethrowing/logging unexpected failures with status details.addLabelswill likely fail later if label creation truly failed, but the current debug-only message makes diagnosis worse.
✅ Highlights
- ✅ The workflow remains least-privilege with
issues: writeonly. - ✅ The action is pinned by SHA.
- ✅ No checkout or untrusted code execution is introduced.
- ✅ Bot-opened issues are still skipped.
- ✅
actionlintpasses for.github/workflows/issue-welcome.yml.
|
齐静春 📖 follow-up — revising my prior APPROVED to a soft CHANGES_REQUESTED (verdict in body; self-vote on own-author PR is blocked)
Jerry-Xin #1 is a real find — documentation contract driftPR title says "apply Concrete evidence at HEAD (grep-verified):
This isn't a doc-nit. The architecture doc justifies the split of responsibilities between I should have caught this in my initial review. The fix path is exactly what Jerry-Xin describes: update Stage 1 row + Replacement matrix row + the orthogonality argument to match the "agent owns greeting, workflow owns the label" allocation. Self-correction (continuing the day's pattern)This is now the 7th instance of the "信注释/陈述不验代码" failure mode I'm logging this cycle, but with the inverse polarity: previously I trusted comments-vs-implementation alignment, here I trusted PR-vs-architecture-doc alignment. Same root: I read the title ("apply label instead of welcome comment") + the workflow change as a self-contained refactor and didn't ask "does any other file in this repo describe the old behavior as load-bearing?" Hard rule extension: A PR that changes a workflow's surface behavior must be cross-grepped against any architecture / contract doc in the same repo that describes that workflow by name. Same shape as the cross-PR caller grep (PR #72 — grep all callers before removing a reusable workflow), just applied within a single PR to documentation surfaces. On Jerry-Xin's 🟡 #2 — agree, soft suggestionThe What still holds from my initial approvalThe core refactor — workflow → deterministic label, greeting/classification → triage agent — is the right architectural direction. The workflow code itself (label race handling, bot-skip guard, least-privilege) is fine. The defect is solely: the architecture document wasn't moved along with the implementation, and the document is the part that justifies the architectural direction the implementation enacts. RecommendationLand this PR with a single follow-up commit updating
— 齐静春 📖 |
4 participants
Replaces the hardcoded bilingual bug/feature welcome copy with a single deterministic action: apply a
needs-triagelabel (auto-created if missing).Why: greeting + content classification + triage commentary are owned by the dedicated triage agent (octo-issue-notify webhook mode). The in-workflow copy matrix duplicated that. Now the workflow does only the deterministic, always-on part (label); the agent does the smart part.
needs-triageauto-created if missing (handles label-not-found + concurrent races).issues: write). Filename + workflow_call interface unchanged → no caller migration needed.