cleanup: strip dead notification chain refs from docs#85
Conversation
Jerry-Xin
left a comment
There was a problem hiding this comment.
Project relevance gate: PASS. This PR documents the Mininglamp-OSS/.github automation platform, but the cleanup is incomplete in the main architecture baseline.
🔴 Blocking
🔴 Critical — docs/workflow-architecture.md still treats the removed notification chain as active architecture. The new banner says these workflows/actions were decommissioned, but the document is explicitly an “acceptance baseline” at lines 15-17, and later sections still prescribe .github/actions/octo-notify, IM mode, webhook mode, and consumers including octo-issue-notify, octo-pr-result-notify, octo-pr-review-feed, and octo-ci-status at lines 180-215. The same stale design continues in the triage flow at lines 280-292 and the replacement matrix at lines 313-323. This would direct future work toward workflows/actions that the PR says were removed org-wide. Please either remove/rewrite these sections for the new cron-driven multica autopilot architecture, or clearly move them into an archived historical section that is not part of the current acceptance baseline.
🔴 Critical — docs/workflow-architecture.md still lists octo-notify plus the four notification workflows as Wave 2 implementation work, and lines 496-515 still show an IM coverage column plus actions/octo-notify as the sole outbound transport owner. These are current-looking implementation and audit tables, not historical notes, so they contradict the 2026-06-27 deprecation banner and the PR’s stated cleanup goal.
💬 Non-blocking
🟡 Warning — docs/onboarding-checklist.md still mentions pr-welcome and pr-review in the pull_request_target gotcha. If those map to removed Plane 6 PR notification/welcome flows, this should be cleaned up in the same pass; if they refer to surviving workflows, consider using exact reusable names to avoid confusion.
✅ Highlights
The deletion of docs/cicd-state-snapshot.md is appropriate for removing a stale point-in-time snapshot, and the onboarding checklist correctly drops the removed IM/project group secret requirements.
Jerry-Xin
left a comment
There was a problem hiding this comment.
Project relevance gate: PASS. Docs-only cleanup on the Mininglamp-OSS/.github automation platform. 3 files, +14/-178 — strips dead references to the decommissioned Plane 6 IM-notification chain. No executable workflow/action/template touched, so zero runtime blast radius.
✅ Verified accurate
docs/cicd-state-snapshot.mddeleted entirely (-161) — a stale point-in-time snapshot built around the now-removedocto-notifychain. Correct to drop.docs/onboarding-checklist.md— correctly removesTRIAGE_WEBHOOK_URL/project_group_id/octo-ci-status/octo-pr-review-feedrequirements, the "Octo — Issue automation" starter template, and theocto-issue-notifytriage-webhook gotcha. Remaining gotchas renumbered cleanly.docs/workflow-architecture.md— adds a 2026-06-27 deprecation banner and updates the Plane 6 catalog row.- Spot-checked head SHA
ec04153: the stripped artifacts are genuinely gone —actions/dir is 404 (compositeocto-notifyremoved), andocto-ci-status/octo-issue-notify/octo-pr-result-notify/octo-pr-review-feedreusables no longer exist under.github/workflows/. The doc correctly keepsissue-welcome.yml, which still lives at head. No over-removal, no stripping of still-live refs.
🟡 Non-blocking follow-up (optional)
workflow-architecture.mduses a banner-over-rewrite approach: the body's Plane 6 catalog, Wave 2 implementation table, IM coverage column,actions/octo-notifytransport, and triage-flow sections are left in place rather than rewritten. The new top banner explicitly scopes all of them as "historical context only — do not use them as a target for new work," which adequately prevents misdirection, so this is not blocking. A future pass could fully archive or rewrite those sections (and tighten thepr-welcome/pr-reviewmention in thepull_request_targetgotcha to exact surviving reusable names) for a cleaner baseline.
Net: an accurate, well-scoped docs cleanup that removes only genuinely-dead references and preserves still-live ones. Approving.
Jerry-Xin
left a comment
There was a problem hiding this comment.
Docs-only cleanup — correctly removes the bulk of the dead notification-chain references and adds a deprecation banner. Approving; the remaining items are non-blocking documentation-completeness notes, not blockers (no runtime/CI impact).
✅ What it does right
- Deletes
docs/cicd-state-snapshot.mdentirely (−161) — the as-built snapshot that inventoried the now-removedocto-issue-notify/octo-ci-status/octo-pr-result-notify/octo-pr-review-feedreusables +octo-notifycomposite. - Trims
docs/onboarding-checklist.md(+5/−16) of dead notification-chain steps. - Adds a top-of-file deprecation banner to
docs/workflow-architecture.md(+9/−1) announcing the chain was decommissioned 2026-06-27.
🟡 Non-blocking follow-ups (docs completeness)
docs/workflow-architecture.md: the new banner flags the chain as removed, but the detailed design/audit sections further down (the Wave-2 implementation table and theocto-notifyoutbound-transport / consumer matrix) still read as current-state prose. The banner mitigates the misleading-ness, but a follow-up could archive or rewrite those sections so they're unambiguously historical. Purely a documentation-accuracy nicety — no functional effect.docs/onboarding-checklist.md: thepull_request_targetgotcha still mentionspr-welcome/pr-review. If those map to removed Plane-6 flows, sweep them too; if they're surviving workflows, use exact reusable names to avoid confusion.
Net: this is a net-negative-178-line dead-reference cleanup that lands the bulk of the doc debt from the #83/#84 reusable removal. The leftover architecture-doc sections are a tidy-up follow-up, not a reason to block a docs PR. Approving.
yujiawei
left a comment
There was a problem hiding this comment.
Code Review — PR #83 (.github) — corrected basis
This supersedes my earlier approval, whose stated reasoning was wrong on one load-bearing point. The verdict is unchanged (APPROVED), but the why matters for the audit trail, and two other reviewers correctly flagged the issue.
Correction to my prior reasoning
My earlier review said this deletion is inherently safe because callers pin @v1 and "v1 is frozen pre-deletion." That premise is false. The v1 tag is an actively-rolled major alias: it was moved to 8feb64b today (2026-06-27 17:08 +0800), three commits behind this PR HEAD. Standard practice for an org .github repo is to advance v1 onto main. The real hazard, correctly raised by the other two reviewers, is: once v1 is re-cut to include this deletion, every live @v1 caller breaks at once with "workflow not found." The merge itself is harmless; the v1 re-cut is the trigger.
So the safety of this change was never intrinsic — it depends entirely on the ordering gate: all caller octo-ci-status.yml files must be removed before v1 is re-cut.
Status of the ordering gate (re-verified at review time)
At the time the two blocking reviews were written, that gate was NOT met — ~11-15 repos still had live @v1 callers (octo-web, octo-fleet, octo-server, octo-lib, etc.). Those reviews were correct then.
The caller-removal PRs have since landed. Re-verified just now, two independent ways:
- Direct contents API, all 19 candidate repos: every
.github/workflows/octo-ci-status.ymlnow returns 404.octo-webandocto-fleet, which were confirmed live ~10 minutes earlier, are now both gone. - Org-wide code search
octo-ci-status.yml@v1(ownerMininglamp-OSS): zero live workflow hits; onlydocs/*.mdprose remains.
The precondition the blocking reviewers demanded ("land the remaining caller removals first, then re-verify 0 live hits") is now satisfied.
Spec compliance
Spec: ✅ — Removes exactly the two intended files (octo-ci-status.yml reusable + octo-notify composite); nothing extra, nothing missing. The octo-notify composite's only consumer in this repo was the reusable being removed in the same PR, so bundling the two deletions is correct. The PR body's "all 27 callers already removed" claim was inaccurate when written, but it is effectively true now and does not affect this diff's correctness.
Code quality
Quality: Approved — Pure deletion, no collateral. No remaining functional reference in-repo. Only finding is P2 stale docs (docs/onboarding-checklist.md, docs/cicd-state-snapshot.md, docs/workflow-architecture.md still mention the removed workflow/action) — non-blocking follow-up.
Verdict
APPROVED, on corrected grounds: the deletion is safe to merge and safe to follow with a v1 re-cut, because all live callers are now removed (verified). If for any reason a caller reappears before v1 is advanced, that re-cut must wait. Recommend re-cutting v1 only after a final 0 live hits check, and a follow-up docs sweep.
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).
.github PR#85 Review Report
Reviewer: Octo-Q (automated review)
PR: #85 — "cleanup: strip dead notification chain refs from docs"
Head SHA: ec0415367737e9b367c5e989cac6dc717d301d8b
State at review: MERGED (post-merge review)
Scope: 3 files, +14 −178 (docs-only cleanup)
1. Verification Summary
| Item | Status | Evidence |
|---|---|---|
cicd-state-snapshot.md deletion safe |
✅ | Point-in-time snapshot (2026-06-08); all still-relevant info (CI-green merge gates §5a) is preserved in workflow-architecture.md §5a. No live operational dependency. |
onboarding-checklist.md edits |
✅ | Drops OCTO_BOT_TOKEN/TRIAGE_WEBHOOK_URL secrets row, project_group_id row, "Issue automation" starter entry, and IM-forward gotcha — all consistent with dead notification chain. Renumbering (3→2 gotchas) correct. |
workflow-architecture.md deprecation banner |
✅ | Clear, dated, names all 6 decommissioned components. Frames sections below as "historical context only." |
workflow-architecture.md Plane 6 table row |
✅ | Updated to issue-welcome only, with inline removal note listing all 5 dead workflows + action. Consistent with banner. |
| Internal doc consistency (banner vs Plane 6 row) | ✅ | Both list the same decommissioned set. Banner includes octo-pr-review-feed-comment (not in table row) — minor asymmetry, non-blocking. |
| No new factual claims introduced | ✅ | All additions are removals or deprecation markers. No risk of introducing incorrect data. |
2. Findings
F1 — P2: Stale starter template octo-issue-automation.yml not cleaned up
Diff-scope: pre-existing (template file lives in .github/workflow-templates/, outside this PR's explicit docs/ scope; not introduced or amplified by this PR).
.github/workflow-templates/octo-issue-automation.yml (still on main) calls octo-issue-notify.yml@v1 — a workflow deleted in the earlier Option D cleanup. The .properties.json still advertises it as "Welcome + add-to-board + IM notify + triage-agent webhook on new issues." Any new repo selecting this template from the GitHub Actions UI will get a workflow that references a non-existent reusable, causing an immediate failure.
The PR correctly removed the "Octo — Issue automation" line from onboarding-checklist.md, but the template file itself is the more impactful dead reference — it's selectable, not just readable.
Recommendation: Follow-up PR to delete octo-issue-automation.yml + .properties.json, or strip the notify: job and update the description to match the two surviving jobs (welcome + add-to-project).
F2 — P2 (note): workflow-architecture.md body retains extensive notification-chain design detail
Diff-scope: pre-existing; the banner correctly marks it as "historical context only."
Sections §4a (Issue Triage redesign), Plane 6 detail, and §9 (source-of-truth audit for actions/octo-notify) still describe the decommissioned notification architecture in full design-spec detail. Someone reading §4a without scrolling to the top banner might mistake it for current design. The banner is adequate mitigation for a docs-cleanup PR, but a future pass could collapse or move these sections to an appendix.
Not a blocker — banner is clear, and architecture docs legitimately preserve design rationale.
3. Recommendations
- Delete or update the
octo-issue-automationstarter template (F1). This is the highest-value follow-up — it's the only remaining actionable reference to the dead notification chain that a user can click-to-adopt. - Consider a future pass on
workflow-architecture.md§4a/Plane 6/§9 to collapse decommissioned sections into an appendix or add section-level "historical" markers (F2, low priority).
4. Additional Observations
docs/triage-policy.md— clean. Referencesissue-welcome.yml(still active); no dead-workflow references.SECURITY.md— clean. No notification-chain references; no policy conflict with deprecation.CONTRIBUTING.md— clean. Referencesdocs/triage-policy.mdandSECURITY.md; no dead-workflow references.docs/octo-board-operations-manual.md— clean. Board operations only; no notification-chain references.profile/README.md— not checked in detail (low-signal surface for this cleanup).
5. Data Flow Traceback
N/A — this is a docs-only PR. No runtime data flows, getters, SQL queries, or state mutations are affected. The "consumed data" here is documentation text, and each removal/addition was verified against the actual state of the decommissioned workflows (confirmed deleted from .github/workflows/ on main).
6. Blind-spot Checklist (R5)
- C1 — Dual-path parity: N/A. No symmetric create/remove paths in a docs-only PR.
- C2 — Control-flow ordering / nested reuse: N/A. No runtime control flow.
- C3 — Authorization boundary ≠ capability boundary: N/A. No permission/jail/tool exposure.
- C4 — Authorization lifecycle / container-member cascade: N/A. No auth changes.
- C5 — build/note ≠ runtime path: N/A. No build artifacts, extensions, or CLI changes.
- C6 — Governance/policy doc self-consistency: ✅ Clear.
- Searched
SECURITY.md,CONTRIBUTING.md,docs/triage-policy.md,docs/octo-board-operations-manual.md— no conflicts with the deprecation banner or removed docs. - The deprecation of Plane 6 notification chain does not conflict with any security policy, disclosure process, or contribution workflow.
triage-policy.mdreferencesissue-welcome.yml(still active) — consistent.- One stale artifact identified (F1:
octo-issue-automation.ymltemplate) — not a policy conflict but an incomplete cleanup.
- Searched
7. Cross-round Blocker Recheck (R6)
N/A — automated review review, no prior rounds.
[Octo-Q] verdict: APPROVE — No P0/P1 findings. Two P2 notes (stale starter template, retained historical design detail) are outside this PR's scope and do not represent regressions. Recommend F1 as a follow-up.
Following Option D cleanup (removal of octo-ci-status/octo-notify/etc.), purge references in docs/: