[antigenius0910]

What problem does this solve?

#1571 was merged at d956672 while its remaining review suggestions were still in flight. This PR applies them: F5, F7, F8, and F9 from the round-1 review on #1571. Without them, three problems remain. An image on a higher Node major than its Dockerfile line reads current. A hung connection can use the job's whole 10-minute timeout. Any open issue with the node-security label, including a hand-labeled one, is treated as the tracker and can get closed. In addition, a maintainer edit to the tracker triggers issue-check.yml, which adds incomplete/needs-triage and a comment.

Part of #1567. Follow-up to #1571. Merge after #1570. The dry-run job from #1571 runs on this PR, and it needs the NODE<major>_VERSION ARGs from #1570.

Discord Discussion URL: https://discord.com/channels/1491295327620169908/1491365158868619404/1562955032012398712

Review Contract

Goal

The daily check opens or updates the issue when any image or pin is on the wrong Node major. It fails fast on a hung endpoint. It only touches the tracker issue it created. Maintainer edits to that issue do not trigger issue-check.

Non-goals

• Treating a missing beta-<agent> tag as a problem (F6). This and the pre-beta-* / openab-pty coverage stay as follow-ups.
• The optional simplifications (F10).

Accepted Residual Risks

• An open node-security issue created before this change has no marker, so the check would ignore it and open a new tracker. Node.js security release pending: 22.23.4 #1575 is such an issue. Merge step: right after merge, before the next scheduled run, prepend <!-- node-security-check --> to Node.js security release pending: 22.23.4 #1575's body so the script adopts it. (Doing it earlier is pointless: the pre-marker script on main rewrites the body daily.)
• The node-security label is reserved for the script's tracker. A hand-labeled issue without the marker is skipped by all three automations; that is accepted, since applying the label needs triage permission.
• A pin whose value is on a different major than its ARG name, such as NODE22_VERSION=24.x, also reads major mismatch. That is the intended signal.

Acceptance Criteria

• shellcheck and actionlint are clean on the touched files.
• DRY_RUN=1 against a pinned Dockerfile.unified exits 0, and the body starts with <!-- node-security-check -->.
• With a stubbed image_node_version, beta-claude on 24.x reports major mismatch and the decision is create or update. On main the same input reports current.
• With a stubbed gh, a marked issue is updated or closed and an unmarked labeled issue is ignored.
• issue-check.yml makes no API calls for an issue labeled node-security.

Follow-ups

• F6, a missing status for absent beta-<agent> tags.
• pre-beta-* and openab-pty coverage.

At a Glance

status_of(node, line)
  ""                        -> not published
  major(node) != line       -> major mismatch   (new; counts as behind)
  node < latest security    -> stale
  otherwise                 -> current

reconcile: open issues with label node-security AND body contains <!-- node-security-check -->
issue-check.yml: return early for label node-security

Prior Art & Industry Research

Not applicable. This is a CI hardening follow-up to #1571.

Proposed Solution

• .github/scripts/node_security_check.sh changes:
  • status_of gains the major mismatch case. The new is_behind helper counts it with stale.
  • All three curl calls use --connect-timeout 10 --max-time 30.
  • MARKER is the first line of the issue body, and reconcile filters the labeled issues on it.
  • The header comment states the Dockerfile contract, and a comment next to LABEL names the two workflows that exempt the label.
• .github/workflows/issue-check.yml returns early for issues labeled node-security.

Why this approach?

Each change is the fix the #1571 review proposed, applied with the smallest edit. The marker makes ownership explicit without needing the bot's author name, so the logic works the same in forks and tests.

Alternatives Considered

• Filter by --author github-actions[bot] instead of a marker. Rejected because it ties the logic to the token identity, and the marker is simpler to test.

Validation

All runs used ubuntu:24.04 (bash 5.2) in Docker against live nodejs.org and ghcr.io data.

• DRY_RUN=1 against a pinned Dockerfile.unified exits 0. The body starts with the marker. stable-* is 22.23.1 stale and beta-* is current.
• Major-mismatch test with stubbed image_node_version:
  • beta-claude on 24.22.1 reports major mismatch, and the decision is create or update. At d956672 the same input reported current.
  • beta-kimi on 22.23.4 reports major mismatch.
• Reconcile test with a gh stub that applies the script's real --jq filter:
  • A marked feat: /close slash command to manually terminate a thread session #42 is updated, or closed when nothing is behind.
  • An unmarked fix: create PR for chart bump instead of direct push #7 leads to create, or to no action when nothing is behind.
  • With both present, feat: /close slash command to manually terminate a thread session #42 is updated.
• issue-check.yml script with a stubbed github. A labeled issue makes no API calls. Without the label, the calls are listComments, createComment, and addLabels.
• shellcheck and actionlint 1.7.7 are clean.

🤖 Generated with Claude Code

[chaodu-obk]

Note

LGTM ✅ - Both round-1 blockers are resolved at 190186ab: the node-security label is now documented as reserved for the script's tracker with an explicit #1575 merge step, and major mismatch has its own title fallback, intro wording, and Next steps line.

What This PR Does

Applies the remaining round-1 suggestions from #1571 to the daily Node.js security check. Images or pins on the wrong Node major are reported as major mismatch and count as behind. Every curl call has connect and total timeouts. The script only manages open node-security issues whose body carries <!-- node-security-check -->. issue-check.yml skips issues labeled node-security.

How It Works

• status_of checks ${1%%.*} != line before the security comparison. is_behind treats stale and major mismatch the same way for the pin and the published-image callers.
• load_latest_security, registry_get, and the ghcr token fetch use --connect-timeout 10 --max-time 30.
• render_issue_body writes MARKER as the first line. reconcile lists up to 100 open labeled issues and selects the first whose body (null-safe) contains the marker.
• issue_title falls back to Node <m> major mismatch when a line is behind but has no security release to name.
• issue-check.yml returns early for the node-security label. Its comment now says this only matters for human edits and relabels, since GITHUB_TOKEN writes do not retrigger workflows.

Round 1 Resolution

Round 1	Status	How
🟡 F1 Split ownership predicate / #1575 orphan	✅ Resolved	Took option 2: the LABEL comment declares the label reserved for this script's tracker and names all three files. The PR body's Accepted Residual Risks now records the accepted hand-labeled case and a merge step: prepend the marker to #1575 right after merge, before the next scheduled run. Doing it earlier would not stick, because the pre-marker script on main rewrites the body daily.
🟡 F2 major mismatch wording and empty title	✅ Resolved	issue_title uses ${LATEST[$m]:-Node $m major mismatch}. The intro now covers "behind ... or on the wrong Node major", and a new Next steps item 2 explains the mismatch fix.
🟢 F5 null body / implicit limit	✅ Taken	(.body // "") and --limit 100
🟢 F6 wording	✅ Taken	LABEL comment names all three files; the issue-check.yml comment states the real scope
🟢 F4 --retry-max-time	Not taken	Optional; a hung endpoint already fails in about 127 s
🟢 F7 silent checker failure	Deferred	Pre-existing from #1571; follow-up

Findings

#	Severity	Finding	Location
1	🟢	Title fallback verified locally for every combination of affected lines	.github/scripts/node_security_check.sh:171-172
2	🟢	Reserved-label decision is documented in code and in the PR contract, with a correctly timed merge step	.github/scripts/node_security_check.sh:17-18, PR body
3	🟢	Mismatch rows now have their own call to action, separate from a patch bump	.github/scripts/node_security_check.sh:153,160
4	🟢	Optional nit: the title prefix still reads "security release pending" for a mismatch-only line	.github/scripts/node_security_check.sh:176

Finding Details

🟢 F1: Title fallback

Ran issue_title from the head script under bash 5.2 with LATEST=([22]=22.23.4 [24]=""). These are the titles it produced:

• only 24 behind: Node.js security release pending: Node 24 major mismatch
• both behind: Node.js security release pending: 22.23.4 / Node 24 major mismatch
• only 22 behind: Node.js security release pending: 22.23.4

The output never ends empty or with /. status_of 24.1.0 22 and status_of 22.9.0 24 both return major mismatch, and is_behind counts it.

🟢 F2: Reserved label and merge step

Declaring node-security as reserved makes the three label-only checks consistent by contract, and triage permission bounds the accepted hand-labeled case. The merge step for #1575 is timed correctly. #1575 currently has no marker, and main still runs the pre-marker script, which rewrites the body with gh issue edit --body-file. A marker added before merge would be overwritten.

🟢 F3: Mismatch call to action

Next steps item 2 tells the operator to fix the ARG NODE<major>_VERSION value or rebuild the image from the correct line, instead of bumping the patch version. Mismatch is no longer conflated with a patch bump.

🟢 F4: Optional title wording

For a mismatch-only line, the title reads Node.js security release pending: Node 24 major mismatch. It is unambiguous, but a neutral prefix such as Node.js base image action pending: would match the new intro wording. Not blocking.

Baseline Check

• PR opened: 2026-10-23 02:52 UTC by @antigenius0910
• Head: 190186abfc244ef2dbd62a8bc22dc282583469cc; base main (not stacked); merge-base 3568d597
• Diff vs base: 2 files, +30 / -12; incremental since round 1 (fab4a9d8..190186ab): 2 files, +12 / -7
• Prerequisites chore(docker): pin Node.js base image via NODE22_VERSION/NODE24_VERSION #1570, chore(ci): add daily node-security-check workflow #1571, docs(release): add SECURITY.md and base image security runbook #1572 are on main
• CI at head: check, dry-run, validate, and Review Contract pass

What's Good (🟢)

• The round-1 fixes are minimal and stay inside the existing structure: is_behind and its callers are unchanged
• major mismatch closes the higher-major gap where an image used to read current
• All issue mutations stay in reconcile after the full assessment
• The PR contract was updated together with the code, so the residual-risk record is accurate again

Addressing External Reviewer Feedback

No external review comments or inline threads exist on this PR at the reviewed SHA.

5. Three Reasons We Might Not Need This PR

1. The marker guards a case that has not happened. Adding the label needs triage permission, and the reserved-label contract already covers hand-labeling. The marker's main value is defense in depth.
2. Two different events still share one tracker. Mismatch and security-stale rows now have distinct instructions, but they still share one issue and one title prefix. A separate alert may fit better long term.
3. The merge step is manual. Correctness right after merge depends on a person editing Node.js security release pending: 22.23.4 #1575 before the next 07:47 UTC run. If that step is missed, a duplicate tracker opens.