Skip to content

fix(ci): externalize hardcoded Project + IM group IDs (W-05/W-06/W-07)#53

Merged
lml2468 merged 1 commit into
mainfrom
fix/w05-externalize-hardcoded-ids
May 31, 2026
Merged

fix(ci): externalize hardcoded Project + IM group IDs (W-05/W-06/W-07)#53
lml2468 merged 1 commit into
mainfrom
fix/w05-externalize-hardcoded-ids

Conversation

@lml2468
Copy link
Copy Markdown
Contributor

@lml2468 lml2468 commented May 31, 2026

Summary

Eliminates all hardcoded GitHub Projects Node IDs and Octo IM group IDs from three reusable workflows.

W-05: auto-add-to-project.yml — Dynamic field resolution

Before: PROJECT_ID, MODULE_FIELD_ID, SPRINT_FIELD were hardcoded as opaque Node IDs. REPO_MODULE mapped repos to hardcoded option IDs. Combined with continue-on-error: true, a Project rebuild would silently break all Module + Sprint assignments with zero visibility.

After:

  • Added project-number (default: 2) and org-login (default: Mininglamp-OSS) inputs, matching reusable-check-sprint.yml pattern
  • "Set Module" step: resolves PROJECT_ID + Module field ID + option IDs by name at runtime via GraphQL
  • "Inherit Sprint" step: resolves PROJECT_ID + Sprint field ID by name at runtime
  • REPO_MODULE now maps repo → module-option-name (e.g. "octo-server": "server") instead of opaque IDs
  • Proper core.warning() calls surface errors in CI logs instead of silent failure
  • continue-on-error: true retained (non-critical ops should not block board addition)

Self-healing: Project rebuild, field delete+recreate, or option recreate — as long as field/option names remain the same, zero manual intervention needed.

W-06: octo-ci-status.yml — Externalize CI status group ID

  • Added ci_status_group_id input with current value as default
  • Replaced hardcoded "4ade985d..." literal in Python script with CI_STATUS_GROUP_ID env var

W-07: octo-issue-feed.yml — Externalize issue feed group ID

  • Added feed_group_id input with current value as default
  • Replaced hardcoded "151a4597..." literal in Python script with FEED_GROUP_ID env var

Backward compatibility

All three changes use defaults matching current production values. No caller changes required. Callers that want to override can pass the new inputs explicitly.

Verification

# No hardcoded Node IDs remain in auto-add-to-project.yml
grep -c "PVT_kwDOEOckHc4BXcvH\|PVTSSF_\|PVTIF_" auto-add-to-project.yml  # → 0

# No hardcoded group IDs remain in script bodies (only in input defaults)
grep "4ade985d" octo-ci-status.yml   # → only in default: field
grep "151a4597" octo-issue-feed.yml  # → only in default: field

W-05: auto-add-to-project.yml
- Add project-number and org-login inputs (matching check-sprint pattern)
- Dynamically resolve PROJECT_ID, Module field ID, and Sprint field ID
  via GraphQL at runtime instead of hardcoding Node IDs
- REPO_MODULE now maps repo→module-name instead of repo→option-ID,
  resolving option IDs at runtime from the project field options
- Project rebuild/field recreate now self-heals as long as names match

W-06: octo-ci-status.yml
- Add ci_status_group_id input with current value as default
- Replace hardcoded group ID literal in Python script

W-07: octo-issue-feed.yml
- Add feed_group_id input with current value as default
- Replace hardcoded group ID literal in Python script

All three changes are backward-compatible: defaults match current
behavior, no caller changes required.
@lml2468 lml2468 requested a review from a team as a code owner May 31, 2026 15:46
Copy link
Copy Markdown
Contributor Author

@lml2468 lml2468 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review — PR #53 (.github) @ 829fd70

Verdict: APPROVED

Significant quality improvement across 3 reusable workflows — externalizes all hardcoded Project V2 + IM group IDs.

1. auto-add-to-project.yml (+98 -27)

Architecture upgrade: replaces 3 hardcoded Project V2 IDs (PROJECT_ID, MODULE_FIELD_ID, SPRINT_FIELD) + 19 hardcoded Module option IDs with runtime GraphQL resolution.

  • New inputs: project-number (default 2), org-login (default "Mininglamp-OSS") — backward compatible
  • REPO_MODULE now maps to human-readable names ('infra', 'adapters', etc.) instead of opaque option IDs — self-healing across project rebuilds
  • Runtime resolution via two GraphQL queries (Module field + Sprint field) with proper null checks and warning messages
  • Error handling includes available options in warning for easy debugging

2. octo-ci-status.yml (+8 -1)

  • Externalized hardcoded CI status group '4ade985d984e432eb7fbdd0ad4f8118a' to input ci_status_group_id with default
  • Uses require_group_id for hex format validation ✓

3. octo-issue-feed.yml (+8 -2)

  • Externalized hardcoded issue feed group '151a45970e1546afa9e947ac36a5c4e5' to input feed_group_id with default
  • Uses require_env for non-empty check

Non-blocking

🟡 octo-issue-feed.yml:156require_env('FEED_GROUP_ID') checks non-empty but doesn't validate 32-char hex format. octo-ci-status.yml uses require_group_id for the same purpose. Consider adding require_group_id to issue-feed for parity (would need to define the function since it doesn't exist in that file).

CI all green. No blocking issues.

Copy link
Copy Markdown

@Jerry-Xin Jerry-Xin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR is in scope for Mininglamp-OSS/.github; it updates reusable workflows owned by this repository. I found no blocking issues.

💬 Non-blocking

  • 🟡 Warning: octo-issue-feed.yml reads FEED_GROUP_ID with require_env() only. For consistency with octo-ci-status.yml and octo-pr-result-notify.yml, consider validating it as a 32-char lowercase hex group ID before sending. This is not blocking because send failures still fail the job and callers already control the supplied secret/input context.

✅ Highlights

  • Dynamic Project V2 ID, field ID, and option ID resolution in auto-add-to-project.yml removes brittle Node ID coupling while preserving existing defaults.
  • The privileged pull_request_target workflow still avoids checkout or execution of PR code.
  • octo-ci-status.yml correctly validates the externalized CI group ID before use.

Copy link
Copy Markdown

@yujiawei yujiawei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review — PR #53 (.github)

Verdict: APPROVED

This is a clean, well-motivated change. It removes opaque hardcoded Project Node IDs / IM group IDs from three reusable workflows and replaces them with runtime name-based resolution (W-05) and externalized inputs with prod-matching defaults (W-06/W-07). I reviewed correctness, security/consistency, and behavioral equivalence. No P0/P1 issues; the items below are P2 nits and notes that do not block merge.

What I verified ✅

  • Backward compatibility holds. All three new inputs are required: false with defaults equal to current production values, so existing 19 callers need zero changes. Omitting the inputs reproduces the previous behavior exactly (orgs/Mininglamp-OSS/projects/2, group IDs 4ade985d… / 151a4597…).
  • REPO_MODULE name migration is faithful. I checked all 19 repos: every new module option name equals the prior // comment for its old opaque option ID, and the partition is preserved — repos that shared an option ID still share a name (e.g. infra ← {.github, community, octo-version-sync}; adapters ← {claw-channel-octo, octo-adapters, openclaw-channel-octo}; cli ← {octo-cli, octo-daemon-cli}; server ← {octo-im, octo-server}). No silent split or merge.
  • The || default fallbacks are correct and load-bearing — not redundant. auto-add-to-project.yml fires on three triggers (workflow_call, issues, pull_request_target). The inputs context is only populated on the workflow_call path; on the issues / pull_request_target direct-trigger paths inputs.org-login / inputs.project-number are empty, so ${{ inputs.org-login || 'Mininglamp-OSS' }} / ${{ inputs.project-number || 2 }} are required for those runs to resolve a valid project URL. Do not remove them — doing so would break the direct-trigger path (orgs//projects/).
  • pull_request_target safety preserved. No actions/checkout of PR head is added, permissions: {} at workflow level, token only via the PROJECT_TOKEN secret. The inline SECURITY comment correctly warns against adding a checkout. Good.
  • GraphQL null-handling is sound. Each resolution step guards !project?.id and !field?.id with core.warning(...) + return; the inline fragment on the wrong field type collapses to a null id and is caught. The not-found option path logs available option names — better diagnostics than the old opaque-ID failure.

Findings (non-blocking)

P2 — W-07 feed_group_id is not format-validated, unlike its W-06 sibling

octo-issue-feed.yml reads the new group id via feed_gid = require_env('FEED_GROUP_ID') (non-empty check only) and does not define require_group_id. octo-ci-status.yml validates the equivalent input with require_group_idre.fullmatch(r'[0-9a-f]{32}', val) else sys.exit(2). Both inputs were externalized in this same PR with identically-shaped 32-hex defaults, so the validation contract should match.

This is a consistency / defense-in-depth gap, not a security hole: the group id is placed in the JSON request body as channel_id (json-escaped — no injection), not in the URL, and the request host is still pinned by the API_BASE_URL allowlist. Worst case from a malformed override is a failed IM send against the allowlisted endpoint — no SSRF/exfil. The shipped default is valid, so the happy path is unaffected.

Suggestion: port require_group_id into octo-issue-feed.yml (or factor a shared snippet) and use it for FEED_GROUP_ID so all three workflows share one validation contract.

P2 (note) — project-number numeric falsy edge

project-number is type: number; in ${{ inputs.project-number || 2 }} an explicitly-passed 0 is falsy and coerces to 2. Practical blast radius is nil (0 is not a valid Project number), so no action required — flagging only because the || idiom masks an invalid 0 rather than surfacing it. If stricter behavior is ever wanted, validate numerically in-script instead of via ||.

Note — resolution failure mode shifts (accepted design)

By moving from opaque IDs to name lookup, the dependency surface shifts: a future rename of a "Module"/"Sprint" option label in the Project UI would make options.find(o => o.name === …) miss and — under the pre-existing continue-on-error: true — skip the assignment while the workflow stays green. This is the intended, reasonable trade (human-readable, survives ID churn) and is already documented in the new code comments. Worth making board admins aware that option labels are now load-bearing. The continue-on-error: true is pre-existing (not introduced here), so this is not a regression.

Summary

Solid IaC hygiene improvement with the self-healing runtime resolution as a genuine robustness win on the ID-churn axis. The only thing I'd nudge before/after merge is aligning W-07's group-id validation with W-06's. Approving.

@lml2468 lml2468 merged commit 4ef61d2 into main May 31, 2026
4 checks passed
@lml2468 lml2468 deleted the fix/w05-externalize-hardcoded-ids branch May 31, 2026 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants