feat(octo-pr-review-feed): add enable_im_notify input + PR_TRIAGE_WEBHOOK_URL secret

[lml2468]

Summary

Phase 2 of the workflow-driven triage migration (Phase 1 was #80, which added the same toggle to `octo-issue-notify`). The PR side of the feed pipeline was previously IM-only; this PR adds:

1. New optional secret `PR_TRIAGE_WEBHOOK_URL` — when set, the sanitized PR payload is POSTed to the URL for downstream fan-out review dispatch.
2. New input `enable_im_notify` (boolean, optional, default `true`) — when `false`, IM push is skipped (webhook still fires).

Change

• Added `enable_im_notify` input (default `true`).
• Added `PR_TRIAGE_WEBHOOK_URL` optional secret.
• Build step now emits `webhook_payload` (same shape as octo-issue-notify).
• Send step now passes `im-message: ${{ inputs.enable_im_notify && steps.build.outputs.im_message || '' }}` (composite action skips IM when message is empty per its docs) and adds `webhook-url` / `webhook-payload` (composite action skips webhook when URL is empty).

Backward compatibility

`default: true` + optional secret preserves existing behavior for all current callers. No-op for any consumer not opting in. Mirrors the Phase 1 pattern exactly.

Test plan

• Merge this PR.
• Move `v1` tag forward (per Phase 1 lesson — caller workflows pin `@v1`).
• Opt one caller (e.g. `octo-version-sync/.github/workflows/octo-pr-review-feed.yml`) into `enable_im_notify: false` and set `PR_TRIAGE_WEBHOOK_URL`.
• Open a draft PR → mark as ready_for_review → expect: workflow runs, IM skipped, `PR_TRIAGE_WEBHOOK_URL` receives one POST, multica PR fan-out autopilot creates one tracked issue.
• Confirm a non-opted-in repo still pushes to IM and sends webhook unchanged behavior wise.

Rollback

Revert this PR; default behavior is unchanged either way.

[OctoBoooot]

Review: feat(octo-pr-review-feed): add enable_im_notify input + PR_TRIAGE_WEBHOOK_URL secret (#81)

Verdict: Approve with comments.

Risk tier: infra (reusable workflow, fan-out across callers). CI green (actionlint + no-tabs). 1 file, +30/-2. The PR-review-feed counterpart to #80, plus it wires the webhook side (PR_TRIAGE_WEBHOOK_URL + payload) in the same PR.

Verification

• Same gate pattern as #80, correct: im-message: ${{ inputs.enable_im_notify && steps.build.outputs.im_message || '' }} — false → empty → IM mode off via the composite action's existing contract. default: true preserves all current callers.
• Webhook plumbing is self-consistent: PR_TRIAGE_WEBHOOK_URL is required: false; webhook-url/webhook-payload are wired into the Send step; webhook_payload is built from already-validated inputs (repo_name, pr_number→int(), sanitized pr_title/pr_author, pr_url, event_action). All six vars are defined (lines 109-114) before the payload uses them — no scope/order bug.
• emit() is heredoc-delimited (key<<DELIM), so the JSON webhook_payload value can't break the $GITHUB_OUTPUT parser even though it contains {/"/:. Safe.
• Inputs are sanitized: pr_title/pr_author go through sanitize_text (author capped at 80 chars) before landing in both the IM message and the webhook JSON. json.dumps handles escaping for the webhook context. No injection into either sink.
• event_action allow-list: only ready_for_review/review_requested proceed; anything else emits skip=true and exits 0. The webhook only fires for the two intended actions.

Minor (same edge as #80; not blocking)

• enable_im_notify: false + unset PR_TRIAGE_WEBHOOK_URL → notify job fails. Identical to the #80 edge: both im-message and webhook-url empty → composite action's neither-mode sys.exit(2) → Send step red. This PR makes the path reachable (IM was previously unconditional). Worth either a step-guard that skips Send when both would be empty, or a one-line note in the enable_im_notify description that false requires PR_TRIAGE_WEBHOOK_URL. (If #80's fix lands one way, mirror it here for consistency.)

Praise

• Bundling the webhook plumbing (PR_TRIAGE_WEBHOOK_URL + payload build) with the IM toggle in this PR — rather than the #80 pattern of toggle-now/webhook-later — means a PR-review-feed caller can go webhook-only in a single coordinated step. Tighter than the two-phase rollout the issue-feed side took.
• The webhook payload is a clean structured contract (repo/number/title/url/author/action) with number as an int, not a string — downstream triage agent gets typed fields, not a pre-formatted IM string it would have to parse back.
• Reuses the exact gate expression and composite-action contract from #80 — consistent mental model across the two feed workflows; an operator who learned the issue-feed toggle already knows this one.

Out of scope (informational)

• PR_TRIAGE_WEBHOOK_URL is a new secret name (distinct from the issue-feed's TRIAGE_WEBHOOK_URL). Intentional — PR-review triage and issue triage are separate agents/endpoints — but worth a one-line callout in any rollout doc so operators don't assume the two feeds share one webhook secret.

[Jerry-Xin]

This PR is in scope for Mininglamp-OSS/.github and the workflow change is consistent with the existing octo-issue-notify Phase 1 pattern.

💬 Non-blocking

🟡 Warning: docs/workflow-architecture.md:191 still documents octo-pr-review-feed as IM-only with webhook mode disabled. After this PR, that table becomes stale and should be updated in a follow-up so the architecture docs match the reusable workflow behavior.

🔵 Suggestion: .github/workflows/octo-pr-review-feed.yml:145 will fail through octo-notify if a caller sets enable_im_notify: false without also passing PR_TRIAGE_WEBHOOK_URL, because neither transport is enabled. That is reasonable as misconfiguration feedback, but it may be worth documenting explicitly near the input description.

✅ Highlights

The PR preserves backward compatibility with enable_im_notify defaulting to true, keeps webhook payload construction inside the mapping workflow, and uses json.dumps rather than string-built JSON. I also ran the repository’s workflow checks locally: actionlint passed and no tabs were found in workflow/action YAML files.

[mochashanyao]

[Octo-Q · automated review]

Verdict: Approve — no blocking findings; notes below (data-flow traced).

---

.github PR#81 Review Report

Reviewer: Octo-Q (automated review)
Head SHA: b6d39675180070ca3077357587049ca556d84d66
Repo: Mininglamp-OSS/.github
File: .github/workflows/octo-pr-review-feed.yml (+30 / -2)
Routing: security_sensitive (automated review)

---

1. Verification Conclusion

Item	Status	Evidence
Phase 1 pattern parity (octo-issue-notify)	✅	Input/secret/ternary/webhook shape identical to octo-issue-notify.yml (main)
enable_im_notify input definition	✅	Lines 34-39: type: boolean, required: false, default: true
PR_TRIAGE_WEBHOOK_URL secret definition	✅	Lines 46-51: required: false with capability-secret description
Webhook payload construction	✅	Lines 130-138: json.dumps() over env vars, safe serialization
IM-message ternary expression	✅	Line 147: `${{ inputs.enable_im_notify && steps.build.outputs.im_message
Backward compatibility	✅	default: true + optional secret preserves behavior for all current callers
Input validation (existing)	✅	require_repo_name, sanitize_text, require_env all upstream of new payload construction
permissions: {} retained	✅	Line 53 — least-privilege preserved

---

2. Findings

F1 — octo-notify errors when enable_im_notify: false AND PR_TRIAGE_WEBHOOK_URL not set

Severity: P2 (configuration-edge) | Confidence: 9/10
Diff-scope: new (introduced by this PR — the enable_im_notify toggle is new; the failure path is a consequence of the new toggle)

Trace:

• octo-pr-review-feed.yml:147 → when enable_im_notify is false, im-message resolves to ''
• octo-pr-review-feed.yml:150 → when PR_TRIAGE_WEBHOOK_URL secret is not configured, webhook-url resolves to ''
• octo-notify/action.yml → both im_message (empty) and webhook_url (empty) skip both transport blocks
• octo-notify/action.yml → attempted = False → sys.exit(2) with error message: "octo-notify invoked with neither im-message nor webhook-url"

Impact: A caller who sets enable_im_notify: false but forgets to set PR_TRIAGE_WEBHOOK_URL gets a hard failure. The error message is clear and actionable, so this is a loud-fail, not silent data loss. However, the documented intent is "only fire the PR triage webhook" — the user's intent is misconfigured, not the code silently doing the wrong thing.

Recommendation: Consider adding a job-level guard before the Send step, e.g.:

- name: Validate transport config
  if: steps.build.outputs.skip != 'true' && !inputs.enable_im_notify && !secrets.PR_TRIAGE_WEBHOOK_URL
  run: |
    echo "::error::enable_im_notify is false but PR_TRIAGE_WEBHOOK_URL is not set. At least one transport must be configured."
    exit 1

This provides a clearer, earlier error with a workflow-specific message. But the current behavior (octo-notify exits 2) is acceptable — not a blocker.

---

F2 — Webhook payload always emitted even when PR_TRIAGE_WEBHOOK_URL not set

Severity: Nit | Confidence: 10/10
Diff-scope: new

emit('webhook_payload', webhook_payload) at line 141 always writes to GITHUB_OUTPUT regardless of whether the secret is configured. The downstream octo-notify action skips webhook when webhook-url is empty (correctly), so the payload in GITHUB_OUTPUT is harmless dead data. Consistent with Phase 1 (octo-issue-notify does the same). No action needed.

---

F3 — import json placed mid-script

Severity: Nit | Confidence: 10/10
Diff-scope: new

Line 130: import json is placed after the emit() calls instead of at the top of the Python script with import os, re, sys. Functionally correct (Python caches imports). Minor style inconsistency with the existing imports at the script top. No action needed.

---

F4 — @v1 tag must be moved forward post-merge (deployment note)

Severity: P2 (deployment-process) | Confidence: 8/10
Diff-scope: pre-existing (applies to all reusable workflow changes in this repo; not introduced by this PR)

PR test plan acknowledges this as a manual step: "Move v1 tag forward (per Phase 1 lesson, caller workflows pin @v1)." If the tag is not moved, callers using octo-pr-review-feed.yml@v1 will use the old version that doesn't accept enable_im_notify or PR_TRIAGE_WEBHOOK_URL, causing workflow_call validation errors when those inputs are passed.

Current action.yml for octo-notify on main already has webhook-url/webhook-payload inputs (Phase 1 landed), so the composite action @v1 already supports webhook mode. The tag-forwarding risk is only for the new enable_im_notify input on octo-pr-review-feed.yml itself.

---

3. Suggestions

1. Job-level transport validation (F1): Add an explicit guard step that errors early with a workflow-specific message when neither transport is configured, rather than relying on octo-notify's generic exit-2.
2. Docs update: docs/onboarding-checklist.md mentions TRIAGE_WEBHOOK_URL but not the new PR_TRIAGE_WEBHOOK_URL secret. Consider updating the secrets table after merge.

---

4. Additional Findings

• OCTO_BOT_TOKEN still required when IM disabled: The workflow declares OCTO_BOT_TOKEN: required: true even when enable_im_notify: false. The token is unused in webhook-only mode but callers must still pass it. Pre-existing constraint (Phase 1 has the same shape). P2 note, not a blocker.
• No webhook URL allowlist: The IM transport has a hardcoded allowlist (ALLOWED_API_BASES), but the webhook transport accepts any URL. This is by design — the URL is a capability secret, and the composite action documents "never hardcode it." Acceptable for this use case.

---

5. Data Flow Tracing

Consumed Data	Upstream Source	Validation	Flows Correctly?
im-message (Send step)	steps.build.outputs.im_message ← Python emit('im_message', message) ← message built from sanitized pr_title (control chars stripped, 300 char cap) + pr_author (80 char cap) + repo_name (regex validated) + pr_url (required env var)	sanitize_text, require_repo_name, require_env	✅ Yes — when enable_im_notify: true; empty string when false
webhook-payload (Send step)	steps.build.outputs.webhook_payload ← Python json.dumps({...}) with same validated env vars	json.dumps() safe serialization; int(pr_number) for numeric coercion	✅ Yes — always emitted; consumed only when webhook-url non-empty
webhook-url (Send step)	secrets.PR_TRIAGE_WEBHOOK_URL — org/repo-level secret	GitHub Actions secret resolution; empty string when not set	✅ Yes — empty when not configured, URL when set
enable_im_notify (Send step ternary)	inputs.enable_im_notify — workflow_call boolean input	default: true; GitHub Actions boolean type validation	✅ Yes — ternary correctly evaluates to message or empty string

---

6. Blind-Spot Checklist (R5)

C1 — Dual-path parity (including guard/lint tool coverage): N/A. This PR adds an optional input and secret to a single reusable workflow. No symmetric paths (add/remove, subscribe/unsubscribe) are affected. The toggle expression mirrors Phase 1 (octo-issue-notify) exactly.

C2 — Control-flow ordering / nested reuse + security control non-canonical test: Clear. The im-message ternary ${{ inputs.enable_im_notify && steps.build.outputs.im_message || '' }} uses standard GitHub Actions short-circuit evaluation. inputs.enable_im_notify is a typed boolean input with a default — it cannot be undefined or a non-boolean string. The webhook-payload is built via json.dumps() (safe against injection into the JSON structure). The webhook URL is passed through env: into octo-notify (never interpolated into shell or JSON). No ordering or nesting issues.

C3 — Authorization boundary ≠ capability boundary: Clear. permissions: {} is retained (least privilege). OCTO_BOT_TOKEN and PR_TRIAGE_WEBHOOK_URL are both secrets. No new endpoints or tools are exposed. The webhook URL is documented as a capability secret.

C4 — Authorization lifecycle / container-member state cascade: N/A. This is a CI/CD workflow, not a multi-tenant authorization system.

C5 — Build/note pass ≠ runtime path correct: Clear. This is a GitHub Actions YAML workflow file. The only "runtime" is GitHub Actions expression evaluation. The ${{ }} expressions are standard patterns (&& / || ternary, steps.x.outputs.y, secrets.Z) validated by Phase 1 parity. No build artifacts, browser extensions, CLI tools, or relative-path resolution involved.

C6 — Governance / policy / security document self-consistency: Clear. This PR only modifies octo-pr-review-feed.yml. No governance, security, or disclosure documents are changed. The existing docs/onboarding-checklist.md and docs/cicd-state-snapshot.md reference this workflow but don't conflict with the new capability. Docs update is recommended (F1 suggestion) but not a consistency blocker.

---

7. Cross-Round Blocker Recheck (R6)

N/A — first review of this PR.

---

Summary

This is a clean, well-structured PR that faithfully mirrors the Phase 1 pattern (octo-issue-notify.yml) to the PR review feed workflow. The +30/-2 diff is minimal, backward-compatible (default: true + optional secret), and uses the same ternary expression shape, webhook payload construction, and composite action delegation. Data flow from env vars through Python validation/sanitization to step outputs to octo-notify inputs is correctly traced. No P0 or P1 issues found. Two P2 notes (configuration-edge error clarity and deployment tag management) and two nits.

[Octo-Q] verdict: APPROVE — no P0/P1 findings; clean Phase 1 parity with backward compatibility.

[yujiawei]

Code Review — PR #81 (.github)

1. Spec / scope compliance

Spec: ✅

The diff implements exactly what the description states (Phase 2 of the workflow-driven triage migration), nothing more, nothing less:

• Done — enable_im_notify input — octo-pr-review-feed.yml:34-42, type: boolean, required: false, default: true.
• Done — PR_TRIAGE_WEBHOOK_URL optional secret — octo-pr-review-feed.yml:46-51, required: false.
• Done — build step emits webhook_payload — octo-pr-review-feed.yml:130-142, json.dumps over the same sanitized fields used for the IM message, same shape as octo-issue-notify (Phase 1).
• Done — send step wiring — octo-pr-review-feed.yml:149,153-154: im-message gated by the enable_im_notify ternary, plus webhook-url / webhook-payload.

No over-build (no extra flags/fields/endpoints), no deviation from the stated approach. The pattern is a faithful mirror of the already-merged Phase 1 PR (octo-issue-notify), down to the identical inputs.enable_im_notify && ...im_message || '' ternary.

2. Code quality

Quality: Approved

Cross-file dependency verified explicitly (the load-bearing claim of this PR): the called composite action octo-notify@v1 already supports webhook mode and the empty-input skip semantics at the tag this workflow pins — empty im-message ⇒ IM skipped, empty webhook-url ⇒ webhook skipped, payload posted via urllib with inputs consumed through env: (no shell / ${{ }} string interpolation). So there is no version-skew window: moving the consuming v1 tag forward lands on a capable action, and the reverse (old workflow + new action) simply passes no webhook inputs.

Security (this PR is security-sensitive):

• PR_TRIAGE_WEBHOOK_URL is declared under secrets: (auto-masked in logs), passed straight into the action's with:, never echoed — correct capability-secret handling, no hardcoded URL.
• Webhook payload carries only sanitized, length-capped PR metadata (repo/number/title/url/author/action) — no token/secret, no OCTO_BOT_TOKEN.
• No injection surface: payload built with json.dumps over python-sanitized values (control-char strip, length caps, repo_name regex, event_action allow-list, 32-hex group id); no shell interpolation.
• Destination URL is an admin-set secret, not derived from untrusted PR input — no SSRF/exfil surface introduced.
• permissions: {} unchanged.

Non-blocking findings

• P2 — enable_im_notify: false with PR_TRIAGE_WEBHOOK_URL unset hard-fails the step (exit 2). octo-pr-review-feed.yml:149,153: when IM is disabled the resolved im-message is ''; if the webhook secret is also omitted, webhook-url is '' too, so octo-notify hits its if not attempted: sys.exit(2) guard. The input's own description ("skip the IM push and only fire the PR triage webhook") presupposes the webhook is configured, so this is a fail-loud guardrail rather than a silent break — defensible behavior. Suggestion (not required for merge): either add a one-line workflow guard that annotates "IM disabled but no webhook configured", or note in the input description that enable_im_notify: false requires PR_TRIAGE_WEBHOOK_URL. The opaque downstream exit 2 is a debuggability gap, not a correctness defect.
• Nit — secrets: inherit callers with a colliding secret name. A caller using secrets: inherit that happens to already hold a secret literally named PR_TRIAGE_WEBHOOK_URL would begin forwarding the webhook without an explicit opt-in. The name is distinctive enough that collision is unlikely; one clarifying clause in the secret description ("only forwarded when explicitly set or inherited") would close the gap.
• Nit — latent a && b || c falsy-middle footgun. The im-message ternary is correct today only because the build step always emits a non-empty message on the non-skip path (skip != 'true' gates the send). If a future change ever let the build emit skip=false with an empty message, an enable_im_notify: true caller's message would be silently coerced to ''. Currently unreachable — flagged only as a maintenance invariant (shared with the merged Phase 1 PR).

3. Overall verdict

APPROVED — Spec ✅ and Quality Approved. No P0/P1. Backward compatibility holds: existing callers (not passing the input, not providing the secret) resolve to a real im-message + empty webhook-url, i.e. exactly the prior IM-only behavior. The three findings above are all non-blocking advisories.

For the human merge decision (security-sensitive)

• Confirm the v1 tag is moved forward after merge (per the test plan) so callers pinned at @v1 pick up the change.
• When a caller is later opted into enable_im_notify: false, ensure PR_TRIAGE_WEBHOOK_URL is set in the same change to avoid the P2 exit-2 footgun.