ci: map octo-docs-html to the server module#89
Conversation
yujiawei
left a comment
yujiawei
left a comment
There was a problem hiding this comment.
Code Review — PR #89 (.github)
Summary
This PR maps octo-docs-html to the existing server Module option in the reusable "auto-add-to-project" workflow, adds a contract test asserting that mapping, and wires the test suite into the Workflow Sanity workflow via a new contracts job (plus .github/tests/** path triggers). Scope is small, focused, and matches the PR description.
1. Spec compliance
Spec: ✅
- Missing work: none. All stated deliverables are present:
octo-docs-html: 'server'added toREPO_MODULE(.github/workflows/auto-add-to-project.yml:89), correctly placed alphabetically.- Contract test
test_octo_docs_html_maps_to_serveradded (.github/tests/test_auto_add_to_project.py:9). - New
contractsjob runs the test suite (.github/workflows/workflow-sanity.yml:27) and.github/tests/**added to bothpull_requestandpushpath filters.
- Out of scope: one unrelated change — removal of a trailing blank line at the end of
auto-add-to-project.yml. Cosmetic and harmless, but strictly unrelated to the mapping change (nit below). - Deviation: none. The
server(rather thanweb) choice is justified —octo-docs-htmlis a Go repository serving an API-only server role, consistent with the siblingocto-*server entries.
2. Code quality
Quality: Approved
Verified locally: python3 -B -m unittest discover -s .github/tests -p "test_*.py" → 1 test, OK.
Strengths:
- New
contractsjob follows the repo's security hygiene: least-privilegepermissions: contents: read,persist-credentials: false, SHA-pinnedactions/checkout@de0fac2…(v6.0.2), and atimeout-minutes: 5. - The test parses the live
REPO_MODULEblock from the workflow YAML rather than hard-coding a duplicate map, so it stays coupled to the source of truth and will catch accidental removal/rename of the entry. - Path triggers are added symmetrically to both
pull_requestandpush, so test changes are actually exercised.
Findings:
- P2 (nit): The trailing-newline removal at the end of
.github/workflows/auto-add-to-project.ymlis unrelated to this change. Not blocking; consider keeping cosmetic churn out of feature PRs. - P2 (nit): The contract test only asserts the single
octo-docs-html → serverentry. That's appropriate for this PR's scope; if the intent is a general mapping contract, a follow-up could assert the full expected map (or at least that all values are known Module option names). No action required here.
3. Overall verdict
APPROVED — Spec ✅ and Quality Approved. No P0/P1 issues; the two items above are non-blocking nits.
mochashanyao
left a comment
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).
Review: .github PR#89 — ci: map octo-docs-html to the server module
Reviewer: Octo-Q (automated review)
Head SHA: 9f9971ed894c403b9f765ffce2d7e8268e186ce0
Base: main
Changed files: 3 (+48/−1)
1. Verification
| Check | Result |
|---|---|
| Mapping entry added in alphabetical order | ✅ octo-docs-html placed between octo-deployment and octo-fleet (.github/workflows/auto-add-to-project.yml:89) |
octo-docs-html is a Go project → server module |
✅ Language breakdown: Go 641K, JS 163K, Shell 7K — consistent with octo-server, octo-fleet, octo-im which all map to server |
| Test file parses the same JS mapping block the workflow uses | ✅ Regex const REPO_MODULE = \{(?P<body>.*?)^\s*\}; correctly captures the block; key-value regex '([^']+)':\s*'([^']+)', matches the single-quote + trailing-comma format used |
| workflow-sanity path triggers updated | ✅ .github/tests/** added to both pull_request.paths and push.paths (lines 11, 16) |
contracts job security posture |
✅ permissions: contents: read, persist-credentials: false, no network calls in test |
| YAML syntax valid | ✅ No syntax issues detected |
2. Findings
No P0/P1 issues found.
P2 — Test covers only the new mapping, not existing ones
- File:
.github/tests/test_auto_add_to_project.py:10–28 - What: The test has a single assertion (
assertEqual(mappings.get("octo-docs-html"), "server")). It does not verify the other 19+ existing mappings. - Impact: If a future edit accidentally deletes or corrupts an existing mapping (e.g.
octo-server), this test would not catch it. - Diff-scope: New — introduced by this PR (new test file).
- Suggestion (non-blocking): Consider parameterising the test to cover all entries, or at minimum a few critical ones (
octo-server → server,octo-web → web,octo-ios → ios). This is a test-robustness note, not a correctness blocker.
P2 — Regex key-value parser is format-coupled
- File:
.github/tests/test_auto_add_to_project.py:19–23 - What: The regex
^\s*'([^']+)':\s*'([^']+)',\s*$assumes single-quoted keys/values with trailing commas. If someone reformats to double quotes or drops the trailing comma on the last entry, the test would see a partial or empty mapping dict and could false-fail. - Impact: False-negative test failure, not a production correctness issue. The workflow itself uses JS (which accepts both quote styles), so the workflow would still work.
- Diff-scope: New — introduced by this PR.
- Suggestion (non-blocking): Acceptable as-is for a single-mapping guard test. If the test is expanded later, consider a more tolerant parser or a YAML-front-matter approach.
3. Data-Flow Trace
Production path (reusable workflow called by octo-docs-html repo):
- Caller repo
octo-docs-htmltriggersworkflow_call github.event.repository.name→ envREPO_NAME="octo-docs-html"(line 73)- JS:
const moduleName = REPO_MODULE["octo-docs-html"]→"server"(line 89, new) - GraphQL query resolves project, Module field, and option where
name === "server"(lines 118–149) - Mutation sets the Module field on the project item (lines 151–165)
continue-on-error: true— failure gracefully skips, does not break the workflow
✅ Every data hop verified; the new mapping integrates correctly with the existing consumption chain. No empty-array / undefined / short-circuit risks.
4. Blind-Spot Checklist (C1–C6)
| # | Check | Result |
|---|---|---|
| C1 | Dual-path parity | N/A — single-directional mapping (repo → module), no symmetric add/remove pair |
| C2 | Control-flow ordering / nested reuse | Clear — the mapping is consumed in a single linear JS block; no nested/duplicate call sites |
| C3 | Authorization boundary | N/A — no auth/permission changes; workflow uses PROJECT_TOKEN secret unchanged |
| C4 | Authorization lifecycle / container-member | N/A — no auth changes |
| C5 | Build/runtime path | N/A — no build artifacts; python3 -m unittest uses stdlib only, available on ubuntu-24.04 |
| C6 | Governance/policy consistency | N/A — CI-only change, no governance docs touched |
5. Extra Notes
- Trailing blank line removal (
auto-add-to-project.yml:320): Cosmetic cleanup of one trailing newline. No functional impact. - Checkout pinning (
actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce8268e186ce0 # v6.0.2): Consistent with existingno-tabsandactionlintjobs. Good.
6. Cross-Round Blocker Re-check (R6)
N/A — automated review review, no prior rounds.
Verdict
[Octo-Q] verdict: APPROVE — Zero P0/P1 findings. Two P2 notes (test coverage breadth, regex format-coupling) are non-blocking test-robustness observations. The production change (one mapping entry) is minimal, correctly placed, and verified end-to-end through the data-flow chain.
lml2468
left a comment
lml2468
left a comment
There was a problem hiding this comment.
Review at head 9f9971ed — ✅ APPROVE
org 级 .github 改动(reusable workflow,组织级 blast radius),但本 PR 极小且安全:把 octo-docs-html → server 加进 project 自动归类的 REPO_MODULE 映射 + 配套契约测试 + 在 workflow-sanity 里跑它。
Gate 1 — 规格符合度 ✅
恰好匹配描述:一行映射(octo-docs-html→server,与其 API-only Go server 角色一致)、一条断言该映射的 Python 契约测试、workflow-sanity 新增 contracts job 跑测试并把 .github/tests/** 纳入触发路径。无过度构建。
Gate 2 — 代码质量(org 级重点:workflow 安全)✅
- 新
contractsjob 安全:pull_request(非_target)、顶层permissions: {}+ jobcontents: read、checkout SHA-pin(de0fac2e…v6.0.2)、persist-credentials: false,只跑仓库自带python3 -m unittest,无${{}}注入不可信 PR 数据、不运行 PR 提供的可执行代码。 - 映射改动零风险:
octo-docs-html: 'server'是REPO_MODULEJS map 里的纯数据项,不触碰 workflow 的 trigger/permissions/注入面。插入位置字母序正确(octo-deployment→octo-docs-html→octo-fleet),值server与同类octo-fleet/octo-im(Go API server)一致。 - 契约测试有效:正则抓
REPO_MODULE块断言octo-docs-html==server,本地python3 -m unittest通过;能防未来误删/改错该映射的回归。 - scope 干净:
git diff核实仅 3 文件;auto-add-to-project.yml的-1是 EOF 多余空行删除,无副作用。 - CI 已全绿(actionlint / Workflow contracts / no-tabs / code-review approved)。
对抗检查(均无问题)
- 新 job 是否给 PR 代码执行/写权限 → 无(
pull_request+permissions:{}+ read-only + 只跑自带测试)。 - 映射改动是否影响
pull_request_target调用方的安全语义 → 无(纯数据,不改控制流/权限/输入处理)。
Verdict: APPROVE — 最小、正确、org-workflow 安全面无回归,契约测试锁定映射,CI 全绿。可合并。
Jerry-Xin
left a comment
Jerry-Xin
left a comment
There was a problem hiding this comment.
The mapping is correct, but the new contract job breaks reusable-workflow callers.
🔴 Blocking
- 🔴 Critical — .github/workflows/workflow-sanity.yml:27: During
workflow_call,actions/checkoutchecks out the caller repository. Most callers will not contain.github/tests, so line 40 fails withImportError: Start directory is not importable: '.github/tests'. Gate this job toMininglamp-OSS/.github, or explicitly check out the workflow repository and appropriate ref.
✅ Highlights
- The
octo-docs-html→servermapping is consistent with the existing mapping structure. - The contract test passes locally.
- Adding
.github/tests/**to the workflow path filters is appropriate.
Superseded: re-posting with repo-relative file citation (dropped an absolute local path).
Jerry-Xin
left a comment
Jerry-Xin
left a comment
There was a problem hiding this comment.
The octo-docs-html → server mapping is correct and purely additive, but the new contracts job breaks every repo that consumes this reusable workflow.
🔴 Blocking
- 🔴 Critical —
.github/workflows/workflow-sanity.yml(newcontractsjob):workflow-sanity.ymlis a reusable workflow (on: workflow_call:) that ~20 org repos call viauses: Mininglamp-OSS/.github/.github/workflows/workflow-sanity.yml@v1(e.g.octo-web,octo-android). Underworkflow_call,actions/checkoutchecks out the caller repo, not.github. Those callers do not have a.github/tests/directory, sopython3 -m unittest discover -s .github/tests -p 'test_*.py'fails withImportError: Start directory is not importable: '.github/tests'(verified: exit code 1 on a repo without that dir). Oncev1is advanced to include this commit, every caller's Workflow Sanity turns red. Thecontractsjob needs to run only for the.githubrepo — gate it withif: github.repository == 'Mininglamp-OSS/.github', or explicitlyactions/checkoutthe.githubrepo at a fixed ref before discovering the tests. (The pre-existingno-tabs/actionlintjobs tolerate the caller tree; this new one does not.)
💬 Non-blocking
- The new
contractsjob pinsactions/checkout@de0fac2e...(SHA) and setspersist-credentials: false/permissions: contents: read— good.
✅ Highlights
- Mapping change is a single additive entry; does not alter any existing repo→module mapping (no regression to other repos).
- The contract test (
test_auto_add_to_project.py) is a pure static-assertion test — no network, no subprocess beyond stdlibunittest. - Adding
.github/tests/**to thepull_request/pushpath filters is appropriate.
5287a9b
yujiawei
left a comment
yujiawei
left a comment
There was a problem hiding this comment.
Code Review — PR #89 (.github)
Re-review at head 5287a9b. The blocking issue raised in the previous round (head 9f9971e) — the new contracts job breaking reusable-workflow callers — has been resolved.
Summary
This PR maps octo-docs-html to the existing server Module option in the reusable auto-add-to-project workflow, adds a Python contract test for that mapping, and wires the test suite into Workflow Sanity via a new contracts job (plus .github/tests/** path triggers). The current head additionally gates the contracts job to the central repository, fixing the caller-breakage flagged previously.
1. Spec compliance
Spec: ✅
- Missing work: none.
octo-docs-html: 'server'added toREPO_MODULE, correctly placed alphabetically betweenocto-deploymentandocto-fleet(.github/workflows/auto-add-to-project.yml:89).- Contract test
test_octo_docs_html_maps_to_serverpresent (.github/tests/test_auto_add_to_project.py:10). - New
contractsjob runs the suite and.github/tests/**added to bothpull_requestandpushpath filters (.github/workflows/workflow-sanity.yml:11,17,27).
- Over-build: none of substance. Only a one-line trailing-blank-line removal at the end of
auto-add-to-project.yml(cosmetic, harmless — nit below). - Deviation: none. The
server(notweb) choice is justified —octo-docs-htmlis a Go, API-only server, consistent with the siblingocto-fleet/octo-im/octo-serverentries.
2. Code quality
Quality: Approved
Verified locally: python3 -B -m unittest discover -s .github/tests -p "test_*.py" → 2 tests, OK. CI on the head is green (No-tabs, actionlint, Workflow contracts all pass).
Previous blocker — now fixed (verified):
- The prior round correctly identified that under
workflow_call,actions/checkoutchecks out the caller repository, which has no.github/tests/directory, sopython3 -m unittest discover -s .github/testswould fail withImportErrorfor every consuming repo oncev1advanced. - Head
5287a9baddsif: github.repository == 'Mininglamp-OSS/.github'to thecontractsjob (.github/workflows/workflow-sanity.yml:29). This is the correct idiom: in a reusable-workflow rungithub.repositoryremains the top-level caller's repo, so the job runs only for.github's ownpull_request/push/workflow_dispatchand is skipped for all callers. A job skipped by a job-levelifis neutral (not a failure), so callers' Workflow Sanity will not turn red. The pre-existingno-tabs/actionlintjobs tolerate the caller tree; the new one now does too, via the gate. - A regression test (
test_contract_job_only_runs_in_central_repository) locks in the presence of this gate — a good guard against the fix being silently removed.
Strengths:
- The
contractsjob follows the repo's security hygiene: least-privilegepermissions: contents: read,persist-credentials: false, SHA-pinnedactions/checkout@de0fac2…(v6.0.2),timeout-minutes: 5, and it runs only the repo's own stdlibunittest(no network, no PR-supplied code execution). - The mapping test parses the live
REPO_MODULEblock from the workflow rather than duplicating the map, keeping it coupled to the source of truth.
Findings:
- P2 (nit): The trailing-newline removal at the end of
auto-add-to-project.ymlis unrelated to this change. Non-blocking; consider keeping cosmetic churn out of feature PRs. - P2 (nit): The mapping test asserts only the single
octo-docs-html → serverentry, and the key/value regex is coupled to the single-quote + trailing-comma format. Both are appropriate for this PR's scope; if this is meant to become a general mapping contract, a follow-up could assert the full expected map (or a few critical entries) with a more tolerant parser. No action required here.
3. Overall verdict
APPROVED — Spec ✅ and Quality Approved. The previously-blocking caller-breakage is resolved and covered by a regression test; the two remaining items are non-blocking nits.
lml2468
left a comment
lml2468
left a comment
There was a problem hiding this comment.
修正:撤回上一轮 APPROVE → REQUEST_CHANGES(live head 9f9971ed)
我上一轮 APPROVE 时漏了 workflow-sanity.yml 是 workflow_call reusable 这一面(我的 trigger 检查没覆盖 workflow_call)。Jerry-Xin 提的 org-wide 回归我 byte 亲核成立,公开纠正——我的 APPROVE 是不完整的审查。
🔴 P1 — 新 contracts job 会 org-wide 打红 ~13 个 caller 仓的 CI
byte 实证 4 点(live head 9f9971ed):
workflow-sanity.yml是 reusable:on: workflow_call:(:5,注释「Allow external repos to call this as a reusable workflow」)。- 被组织仓库当
@v1消费:gh api核实 octo-server / octo-web / octo-android 的.github/workflows/workflow-sanity.yml均uses: Mininglamp-OSS/.github/.github/workflows/workflow-sanity.yml@v1(还有 ~12 个同类)。 - 新
contractsjob 无仓库门控:sed核实该 job 没有if: github.repository == 'Mininglamp-OSS/.github',无条件 checkout + 跑python3 -m unittest discover -s .github/tests。reusableworkflow_call下actions/checkout默认 checkout 的是调用方仓库,不是.github。 - 调用方仓库没有
.github/tests/:gh api实测 octo-server / octo-web / octo-android / octo-cli / octo-ios 全 404。unittest discover -s .github/tests对不存在的目录报ImportError: Start directory is not importable→ exit 1。
后果:一旦 @v1 tag 前进到本 commit,所有没有 .github/tests/ 的 caller 仓(~13 个)的 Workflow Sanity CI 全变红——这不是本仓自测,是推给全组织的回归。两个 peer APPROVE(yujiawei / mochashanyao)漏了这条。
修法(任一):
- 给
contractsjob 加if: github.repository == 'Mininglamp-OSS/.github'(只在本仓跑,最简);或 - job 内显式
actions/checkout.github仓(repository: Mininglamp-OSS/.github)再discover;或 discover前判.github/tests存在再跑(caller 无该目录则跳过)。
建议加一条覆盖「caller 无 .github/tests 时不 fail」的验证。
映射改动本身(安全,无需改)
auto-add-to-project.yml 加一行 'octo-docs-html':'server':纯附加、不改其它 repo 映射、module name 是静态 dict key → 参数化 GraphQL(无注入)、字母序正确、值与同类 Go server 一致。契约测试本身也没问题。问题只在新 contracts job 的无门控 reusable 消费面。
我为何上轮漏了
我把 contracts job 当成「本仓 pull_request 触发的自测」审(那个视角下它确实安全),没检查 workflow-sanity.yml 同时是 workflow_call reusable、会在 caller 上下文运行。这条 org-wide blast radius 是我该在 org .github 仓优先查的,漏了。
Verdict: REQUEST_CHANGES — 未门控的 contracts job 会让 ~13 个消费 @v1 的仓库 CI 变红(caller 缺 .github/tests/ → discover ImportError)。修法明确、contained(加 if: github.repository 门控 + 验证),修好即可复审转 APPROVE。
致谢 Jerry-Xin 抓到这条 reusable-consumer 回归——我漏了 workflow_call 面。
lml2468
left a comment
lml2468
left a comment
There was a problem hiding this comment.
更新:head 已推进到 5287a9bc,blocker 已修 → ✅ APPROVE(撤回我刚才的 CR)
我上一条 CR 锚在 9f9971ed(org-wide contracts job 无门控 → 会打红 ~13 个 caller 仓)。作者在 5287a9bc 已按建议修好,我 byte 复核确认,改判 APPROVE。
🔴→✅ contracts job 现已仓库门控
workflow-sanity.yml 的 contracts job 加了 if: github.repository == 'Mininglamp-OSS/.github'——只在中央 .github 仓运行,caller 仓(octo-server/web/android… 经 @v1 消费)不再触发该 job,unittest discover -s .github/tests 不会在缺 .github/tests/ 的 caller 上跑 → org-wide ImportError 回归消除。
回归被钉死
新增契约测试 test_contract_job_only_runs_in_central_repository——正则抓 contracts job 体、断言含 if: github.repository == 'Mininglamp-OSS/.github',防止门控被误删。本地 python3 -m unittest discover → 2 tests OK。
其余仍成立
- 映射
'octo-docs-html':'server'仍在、字母序正确、值与同类 Go server 一致(纯数据、无注入)。 - job 仍
permissions: contents:read+ checkout SHA-pin +persist-credentials:false;workflow_call/pull_request触发面无其它风险。
Verdict: APPROVE — 我 CR 指出的未门控 reusable 消费回归已在 5287a9bc 修好(if: github.repository 门控 + 契约测试锁定),映射改动本身一直安全。可合并。
(说明:我那条 CR 与作者这次 push 撞车——CR 锚到了同一个 5287a9bc 但内容是针对修复前状态的,以本条 APPROVE 为准。)
mochashanyao
left a comment
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).
Code Review — PR #89 (.github)
Reviewer: Octo-Q (automated review)
Head SHA: 5287a9bccb4b001cf3c5188ca1e7f57bfadf6f34
Summary
This PR maps octo-docs-html to the server module in the auto-add-to-project workflow's REPO_MODULE lookup table, and adds a CI contract job to prevent future mapping regressions. The new contracts job in .github/workflows/workflow-sanity.yml runs Python unittests that verify both the mapping entry and the repository guard on the contracts job itself. Clean, well-scoped change with good test coverage.
Verification
Local checks at head 5287a9b:
-
python3 -m unittest discover -s .github/tests -p 'test_*.py'— pass (2 tests) -
Regex correctness: independently verified both test regexes against the actual workflow files.
REPO_MODULEregex captures all 21 mapping entries;octo-docs-htmlresolves toserver. Contracts-job regex correctly captures the job body and stops at the next job boundary (no-tabs:). -
✅ CI security posture — no
pull_request_targettrigger; new job usespull_request/push/workflow_callonly. Globalpermissions: {}at workflow level, job-levelcontents: read. No secrets consumed by the test step. -
✅ Checkout hygiene —
actions/checkout@v6.0.2pinned to SHAde0fac2ewith version comment;persist-credentials: false. -
✅ Repository guard —
if: github.repository == 'Mininglamp-OSS/.github'ensures the contracts job only runs in the central repo (where the test files and workflow files coexist on the filesystem). Correctly prevents execution when called viaworkflow_callfrom other repos. -
✅ Mapping correctness —
octo-docs-htmlinserted in alphabetical order betweenocto-deploymentandocto-fleet, mapped toserver(consistent with other backend infrastructure repos likeocto-fleet,octo-im,octo-server). -
✅ Test design — tests parse the actual YAML files rather than duplicating data, so a mapping drift or accidental deletion will fail CI. The contracts-job test validates the repository guard is present, preventing accidental removal of the guard that would cause the job to run in fork contexts.
-
✅ Concurrency — existing
workflow-sanity-${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }}group withcancel-in-progress: truecovers the new job correctly. -
✅ Path triggers —
.github/tests/**added to bothpull_requestandpushpath filters, ensuring the contracts job runs whenever test files change. -
✅ Trailing newline cleanup — removed one blank line at EOF of
auto-add-to-project.yml; purely cosmetic, no functional impact.
Findings
No P0/P1 issues. No P2 issues. No correctness, security, build, or functional concerns.
Data Flow Trace
REPO_MODULEmapping in.github/workflows/auto-add-to-project.yml:89— new entry'octo-docs-html': 'server'correctly flows into themoduleNamevariable at line ~103 viaREPO_MODULE[repoName]. WhenmoduleNameis truthy, it's used to resolve the project field option ID. Data path is complete.- The
contractsjob at.github/workflows/workflow-sanity.yml:27reads workflow files from the checkout (viaactions/checkout), passes them topython3 -m unittest discover. Test files read from.github/tests/on disk. Data flow: checkout → filesystem →Path.read_text()→ regex parse → assertion. Complete and correct.
Blind Spot Checklist (C1–C6)
- C1 (dual-path parity): N/A — no symmetric add/remove paths changed.
- C2 (control-flow ordering): N/A — no reused logic across multiple call sites.
- C3 (authorization boundary): N/A — no new endpoints or tool exposure.
- C4 (authorization lifecycle): N/A — no auth changes.
- C5 (build/runtime divergence): Clear —
python3 -m unittestruns identically in CI (ubuntu-24.04) and locally. Verified locally. - C6 (governance/policy): N/A — no policy or governance documents changed.
Cross-round Blocker Recheck (R6)
N/A — first review of this PR.
Verdict: APPROVED
Straightforward mapping addition with well-designed regression tests and a properly secured CI job. All GitHub Actions security best practices are followed. No concerns.
[Octo-Q] verdict: APPROVE — no P0/P1/P2 findings; mapping is correct, CI job is properly guarded, tests are well-designed and pass locally.
Fixed at head 5287a9b: the contracts job is now gated with if: github.repository == 'Mininglamp-OSS/.github', so reusable workflow_call consumers no longer run the caller-breaking discover. Re-anchoring APPROVE at the live head.
Jerry-Xin
left a comment
Jerry-Xin
left a comment
There was a problem hiding this comment.
Summary: The org-wide regression is fixed exactly as prescribed — the contracts job is now gated to run only in the central repo, so reusable workflow_call consumers are no longer broken. Approving.
✅ Highlights (blocker resolved — byte-verified at this head)
contractsjob gated to the central repo..github/workflows/workflow-sanity.ymlnow carriesif: github.repository == 'Mininglamp-OSS/.github'on thecontractsjob. In a reusableworkflow_callinvocationgithub.repositoryis the caller's repo, so for the ~15 repos that consume this workflow via@v1, theifevaluates false and the job is skipped — it no longer checks out the caller and runsunittest discover -s .github/testsagainst a directory those repos don't have. The prior fail-path (caller CI going red once@v1advances) is closed.- Only
contractsneeded gating, and only it was gated. The siblingno-tabs/actionlintjobs run generic linters over workflow files that every caller has, so they don't depend on.github/tests/and are safe underworkflow_callunchanged. - Regression-proof. The added
test_contract_job_only_runs_in_central_repositoryasserts theif: github.repository == 'Mininglamp-OSS/.github'guard is present on the contracts job, so the gate can't be silently dropped later. - Mapping change remains clean & additive.
octo-docs-html → serverin theREPO_MODULEmap is pure data (parameterized GraphQL, no shell/injection), correctly alphabetized, value matches siblings; the reusable'spull_request_targetrisk surface is untouched. SHA-pinned actions,permissions: {}, no secret/infra leak.
No blocking items remain.
8f56626
lml2468
left a comment
lml2468
left a comment
There was a problem hiding this comment.
Re-review at head 8f56626a — ✅ APPROVE(重锚)
新 commit 8f56626a(docs(ci): explain central contract job guard)相对我上轮 APPROVE 的 5287a9bc 只加了一行注释:
+ # Reusable callers check out their own repository, which has no central tests.
if: github.repository == 'Mininglamp-OSS/.github'
byte 核实 delta = 纯文档说明,无行为改动:
contractsjob 的仓库门控if: github.repository == 'Mininglamp-OSS/.github'仍在(workflow-sanity.yml:30)——org-wide 回归依然被挡(reusable caller 上if判 false → job 跳过,不在缺.github/tests/的 caller 仓跑 discover)。- 映射
'octo-docs-html':'server'仍在;契约测试(含test_contract_job_only_runs_in_central_repository门控断言)本地python3 -m unittest discover→ 2 tests OK。 - 那行注释正好把「为何需要这个 guard」写进代码旁,提升可维护性,是好补充。
我上轮在 5287a9bc 的 APPROVE 理由全部继续成立(门控修复 + 回归测试 + 映射纯数据无注入 + SHA-pin + permissions 最小)。本 commit 未触碰任何安全面。
Verdict: APPROVE — 仅新增解释性注释,门控/映射/契约测试均不变且验证通过。可合并。
mochashanyao
left a comment
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).
Code Review — PR #89 (.github)
Reviewer: Octo-Q (automated review)
Head SHA: 8f56626a2b777304a9349b9bf057cb8ec0f2d4f1
Summary
This PR adds octo-docs-html to the REPO_MODULE mapping table in the reusable auto-add-to-project workflow, assigning it to the server module. It introduces a new contracts job in the workflow-sanity pipeline that runs contract tests validating the mapping, and extends the sanity workflow's path triggers to include .github/tests/**. A trailing blank line in auto-add-to-project.yml is cleaned up. The changes are minimal, well-structured, and follow existing conventions.
Verification
Static analysis only at head 8f56626a2b777304a9349b9bf057cb8ec0f2d4f1; build and tests not executed in this environment.
- ✅ Mapping correctness —
octo-docs-htmlis a Go repository (confirmed via GitHub API), consistent with otherserver-mapped repos (octo-server,octo-fleet,octo-im). The PR's claim of "API-only Go server" holds. - ✅ Alphabetical insertion —
octo-docs-htmlplaced betweenocto-deploymentandocto-fleet— correct lexicographic order. - ✅ Column alignment — New entry
'octo-docs-html': 'server'(17-char key + 8 spaces = 25 chars before value) matches the existing alignment convention used by all other entries. - ✅ Test regex #1 (mapping) —
r"const REPO_MODULE = \{(?P<body>.*?)^\s*\};"withMULTILINE|DOTALLcorrectly captures the JS object block; innerr"^\s*'([^']+)':\s*'([^']+)',\s*$"correctly extracts key-value pairs from the single-quoted format used in the file. - ✅ Test regex #2 (contracts guard) —
r"^ contracts:\n(?P<body>.*?)(?=^ [a-z][a-z0-9-]*:\n)"correctly matches fromcontracts:to the lookahead atno-tabs:. Theif:assertion string matches the actual YAML. - ✅ contracts job guard —
if: github.repository == 'Mininglamp-OSS/.github'correctly restricts the job to the central repo, preventing execution when called as a reusable workflow from child repos. - ✅ Permissions —
contents: readis minimal and appropriate for a test-only job. - ✅ Checkout SHA pinning —
actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2matches the version used byno-tabsandactionlintjobs in the same file. - ✅ Path triggers —
.github/tests/**added to bothpull_requestandpushtrigger paths — consistent with existing.github/workflows/**and.github/actions/**entries. - ✅ Concurrency — The new
contractsjob inherits the existing concurrency group; no conflict withno-tabsoractionlint.
Findings
No P0/P1 issues. One P2 observation and one nit below.
P2 — Test regex fragile to job reordering (.github/tests/test_auto_add_to_project.py:29)
The test_contract_job_only_runs_in_central_repository regex uses (?=^ [a-z][a-z0-9-]*:\n) as a lookahead to terminate the contracts job body capture. This requires a subsequent job header to exist after contracts. If contracts were ever moved to be the last job in the file, the lookahead would never match and the test would falsely report "contracts job is missing". Not a correctness bug today (since no-tabs follows), but worth either using end-of-file as an alternative terminator or adding a comment noting the ordering dependency.
Nit — Trailing newline removal (auto-add-to-project.yml:320)
The diff removes a trailing blank line at the end of the file. Cosmetic cleanup — fine.
Data-Flow Trace
REPO_MODULE[repoName]lookup at runtime consumesgithub.event.repository.nameviaprocess.env.REPO_NAME. When a PR/issue originates fromocto-docs-html, the name resolves to"octo-docs-html", which now maps to"server". The downstream GraphQL field-resolution logic is unchanged by this PR (pre-existing path).- The
contractsjob reads the workflow YAML file from disk at CI time viaWORKFLOW.read_text()andSANITY_WORKFLOW.read_text(). The regex extraction operates on the file as checked out, which is the HEAD of the PR branch ormain— the data flows correctly from source file to test assertion.
Blind-Spot Checklist
- C1 (dual-path parity): N/A — no symmetric add/remove or subscribe/unsubscribe paths.
- C2 (control-flow ordering): N/A — mapping is a flat lookup table, no ordering-dependent control flow.
- C3 (authorization boundary): N/A — no auth/permission changes beyond
contents: readon the test job. - C4 (authorization lifecycle): N/A.
- C5 (build/runtime parity): Clear — the
python3 -m unittest discovercommand matches the PR's documented test command. Theactions/checkoutSHA is pinned and matches sibling jobs. - C6 (governance/docs self-consistency): N/A — CI-only change, no governance docs affected.
Cross-Round Blocker Recheck (R6)
N/A — first review of this PR.
Verdict: APPROVED
Clean, well-scoped CI change. The mapping addition is correct (Go server → server module), the contract tests validate the mapping and the central-repo guard, and the workflow-sanity integration follows existing conventions. The P2 regex fragility is a minor maintainability note, not a blocker.
[Octo-Q] verdict: APPROVE — No P0/P1 issues. Mapping is correct, tests validate the contract, CI integration follows conventions. Only a minor P2 regex fragility observation.
Jerry-Xin
left a comment
Jerry-Xin
left a comment
There was a problem hiding this comment.
Summary: Re-anchoring APPROVE at the live head — the only change since my prior approval is an explanatory comment above the repository gate; the fix and mapping are unchanged and correct.
✅ Highlights
- Gate intact + now documented. The
contractsjob retainsif: github.repository == 'Mininglamp-OSS/.github', and the new commit only adds a clarifying comment ("Reusable callers check out their own repository, which has no central tests.") explaining why the guard exists. No logic change — the org-wideworkflow_callregression stays closed (caller repos skip the job that wouldunittest discover -s .github/testsagainst a directory they lack). - Regression-proofed: the contract test asserting the gate is present remains.
- Mapping change unchanged & clean:
octo-docs-html → serveris pure data (parameterized GraphQL, no injection), correctly alphabetized, SHA-pinned actions,permissions: {}, no secret/infra leak.
No blocking items.
yujiawei
left a comment
yujiawei
left a comment
There was a problem hiding this comment.
Code Review — PR #89 (.github)
Reviewed at head SHA 8f56626a2b777304a9349b9bf057cb8ec0f2d4f1.
Summary
Small, well-scoped CI change with three parts:
- Map
octo-docs-html→serverin theREPO_MODULEtable of the reusableauto-add-to-project.yml. - Add a Python contract test (
.github/tests/test_auto_add_to_project.py) asserting the mapping and the guard below. - Add a
contractsjob toworkflow-sanity.yml, guarded so it only runs in the central.githubrepository, and extend the sanity triggers to.github/tests/**.
Spec compliance
- Missing: none. All three stated goals are implemented.
- Over-build: none functional. One unrelated cleanup (removal of a trailing blank line at the end of
auto-add-to-project.yml) is outside the PR's stated scope but is harmless. - Deviation: none.
Correctness
- Module classification is correct.
octo-docs-htmlis a single static Go binary backed by PostgreSQL exposing a versioned/v1HTTP API (per itscmd/+internal/+migrations/layout and README). Mapping it toservermatches its siblings (octo-server,octo-im,octo-fleet). Thewebbucket would be the wrong fit despite the "docs-html" name. - The reusable-workflow guard is necessary and correct. In a workflow invoked via
workflow_call,github.repositoryresolves to the caller's repository. Soif: github.repository == 'Mininglamp-OSS/.github'skips thecontractsjob for the 20+ downstream callers of this reusable workflow — which is exactly right, since a caller's checkout would not contain.github/tests/, andunittest discoverwould otherwise fail there. Running the tests against the central repo only is the intended behavior. The added test that asserts this guard exists is a sensible regression lock. - Trigger extension is right. Adding
.github/tests/**to thepull_request/pushpath filters only affects the central repo's own triggering (path filters don't apply toworkflow_call), so edits to the contract tests now correctly (re)run the sanity workflow. - Mapping insertion keeps the table alphabetically ordered and preserves formatting.
Security
- Workflow-level
permissions: {}with thecontractsjob scoped tocontents: read;persist-credentials: falseon checkout; action pinned by SHA. No secrets required for the contract job. Clean.
Verification performed
- Ran the contract suite at head SHA: both tests pass (
Ran 2 tests ... OK). - Validated both modified workflow files parse as YAML.
- Confirmed
octo-docs-htmlrole via its repository structure and README. - Confirmed the reusable workflow is called by many downstream repos, validating the guard's rationale.
Nits (non-blocking)
- The trailing-newline removal at the end of
auto-add-to-project.ymlis unrelated to the mapping change; fine to keep, just noting the scope.
Coverage note
This review is a single independent pass over the full diff plus the surrounding workflow context and the target repo's role. No areas were left unexamined for a change of this size; the load-bearing element (the reusable-workflow guard) was traced against workflow_call semantics rather than taken on faith.
Verdict
No blocking (P0/P1) issues. Correct, tested, and safe.
5 participants
What
Map
octo-docs-htmlto the existingserverModule option in the reusable project workflow.Why
PRs and issues from the repository need deterministic Module metadata on Octo Board.
How
octo-docs-htmltoserver, matching its API-only Go server role.Checklist
mainTesting
python3 -B -m unittest discover -s .github/tests -p "test_*.py"Breaking Changes
N/A