fix(pr-feed): wire feed_group_id as required input, remove hardcoded group ID#18
Conversation
…group ID
- Add feed_group_id as required input (no default, breaking change intentional)
- Expose FEED_GROUP_ID env var in notify step
- Replace hardcoded '1c303c14...' with require_env('FEED_GROUP_ID')
Callers must now pass feed_group_id explicitly.
Part of PR-feed security & maintainability improvement series.
Co-authored-by: Octo助理-小明 <octo-bot@mininglamp.com>
yujiawei
left a comment
There was a problem hiding this comment.
Code Review — PR #18 (.github)
Summary
Clean, well-scoped change that externalizes a hardcoded feed group ID into a required workflow input. The approach is consistent with how project_group_id is already handled and improves maintainability.
Findings
✅ Correctness
- Input declaration (
feed_group_id, type string, required) is correct YAML. - Env var wiring (
FEED_GROUP_ID: ${{ inputs.feed_group_id }}) follows the established pattern. - Python side:
feed_gid = require_env('FEED_GROUP_ID')+send(feed_gid, feed_msg)correctly replaces the hardcoded call. - Fail-fast behavior preserved via
require_env— callers that forget the input get a clear error at runtime.
✅ Improved ordering
The new code resolves both feed_gid and proj_gid via require_env before any send() call. This is better than the original (which called send with the hardcoded value before resolving proj_gid) — now all validation happens upfront before side effects.
⚠️ Deployment coordination (low risk, informational)
The v1 tag is currently a floating lightweight tag that tracks main (both at 6db6243). All 8 caller repos reference @v1:
uses: Mininglamp-OSS/.github/.github/workflows/octo-pr-feed.yml@v1
If this PR is merged to main and v1 is moved forward before Phase 2 caller updates land, all PR feed notifications will fail with:
ERROR: required environment variable FEED_GROUP_ID is missing or empty
Recommendation: Either (a) hold the v1 tag move until Phase 2 callers are merged, or (b) merge Phase 1 and Phase 2 in a coordinated window. The PR body acknowledges this as intentional — just flagging the operational risk explicitly.
✅ Style / hygiene
- YAML 2-space indentation: consistent.
- Python 10-space inline indentation: consistent.
- Input placed logically after
api_base_url(last optional → first new required). - Env var placed in sensible position within the env block.
Verdict
Approve. The change is correct, minimal, and well-documented. The breaking-change strategy is explicitly acknowledged in the PR body with a clear Phase 2 plan. No code issues found.
## Summary Phase 2 of the pr-feed security & maintainability improvement series. ## Changes ### `.github/workflows/octo-pr-feed.yml` - **`pull_request_target` → `pull_request`** (P0 security fix): eliminates the risk of secrets exposure via malicious fork PRs - **Pin reusable workflow to commit SHA** `db17008c` instead of mutable `@v1` tag - **Add `feed_group_id` input** (required by Phase 1 breaking change in `.github` PR #18) - **Skip draft PRs** with `if: ${{ !github.event.pull_request.draft }}` ## Related - Phase 1 (`.github` PR #18): Mininglamp-OSS/.github#18 Co-authored-by: Octo助理-小明 <octo-bot@mininglamp.com>
Summary
Phase 1 of the pr-feed security & maintainability improvement series.
Changes
.github/workflows/octo-pr-feed.ymlfeed_group_id— callers must now pass the feed channel group ID explicitly; the hardcoded value has been removed (intentional breaking change, callers will be updated in Phase 2)FEED_GROUP_IDenv var wired into the notify stepsend('1c303c14...', feed_msg)withsend(feed_gid, feed_msg)wherefeed_gid = require_env('FEED_GROUP_ID')Why
The feed group ID (
1c303c142e9840f2a9b46c10b0972e8d) was hardcoded in the reusable workflow, making it impossible for callers to override and coupling the central workflow to a specific runtime configuration.Breaking Change
All caller workflows must add
feed_group_id: '1c303c142e9840f2a9b46c10b0972e8d'to theirwith:block. Phase 2 PR will handle all 8 caller repos simultaneously.Checklist