platform-schema: CI hardening, anti-drift hardening, owner-model docs, and trigger coverage - #1462
Conversation
…dance Documents that CI (once openabdev#1337 lands) catches syntactic drift; semantic drift (stale note/status) remains a PR-review concern with no dedicated per-platform owner. Fixes openabdev#1339.
… path traversal Also documents the decision to keep feature sources code-ref-only for now. Fixes openabdev#1340.
…es + weekly safety net Expands path triggers to crates/openab-gateway/src/**, crates/openab-core/src/**, and src/main.rs (all current schema code-ref targets), plus a weekly scheduled run as a backstop. Fixes openabdev#1337.
…spatch and absolute-path test - concurrency group now includes github.event_name — a scheduled run and a push-to-main run both resolve github.ref to refs/heads/main, so without this they'd share a group and cancel-in-progress could cancel one in favor of the other, defeating the weekly safety net. - add workflow_dispatch alongside schedule, matching every other schedule-triggered workflow in this repo (manual test/trigger path). - add check_code_ref_rejects_absolute_path: Path::join replaces the base entirely for an absolute joined path, a distinct code path from the existing '../' traversal test.
|
Note LGTM ✅ - The CI hardening, code-ref validation, and platform-schema maintenance guidance are correctly scoped and covered by passing checks. What This PR DoesThis PR strengthens the platform-schema conformance workflow so code-reference drift is checked when its current target trees change, adds a scheduled backstop, and hardens code-ref validation against empty symbols and paths outside the repository. It also documents the current contribution and semantic-drift maintenance model. How It WorksThe workflow now watches the current code-ref target trees, runs weekly and on demand, uses the repository's checkout/cache conventions, and avoids cross-event cancellation. Findings
Finding Details🟢 F1: Complete current trigger coverageThe local review extracted all current schema Rust code-ref files. Their target directories are contained by 🟢 F2: Robust code-ref validation
🟢 F3: Clear maintenance boundariesThe documentation distinguishes syntactic drift that CI can detect from semantic drift that requires review, while allowing contributors to make direct, scoped schema corrections. Baseline Check
What's Good (🟢)
Addressing External Reviewer FeedbackNo external review comments or inline threads were present when this round began. Reviewer Summary
5. Three Reasons We Might Not Need This PR
|
There was a problem hiding this comment.
Note
LGTM ✅ - The review found no actionable regressions.
Consolidated review: #1462 (comment)
PR Review — 司馬Doc Issues🟢 🟢 🟡
兩段語氣與對象略有重疊,可考慮合併為一段或加小標,讓意圖更清晰。非 blocker。 🟢 Cross-doc consistency Summary
整體:無 blocking doc issue。 |
What problem does this solve?
Closes #1337
Closes #1338
Closes #1339
Closes #1340
Discord Discussion URL: https://discord.com/channels/1491295327620169908/1491365158868619404/1532377620241449040
All four issues were identified during group review of #1295 (the platform-schema knowledge base PR) and are bundled here since they're small, related follow-ups to that same subsystem.
Review Contract
Goal
platform-schema-conformance.yml's trigger paths to also cover the adapter/core source trees that schema code-refs point into, plus a weekly scheduled run as a backstop.actions/checkout@v6(was@v4, inconsistent with rest of repo), addSwatinem/rust-cache, add aconcurrencygroup.check_code_refincrates/platform-schema/tests/conformance.rs: reject empty#symbol, reject path traversal outside repo root.Non-goals
feature_sources_exist_in_treeto skip URL sources like the quirk check does) is intentionally not included — see "Alternatives Considered".Accepted Residual Risks
feature_sources_exist_in_treestill hard-errors on a URL source instead of skipping it (unlike quirks). Verified no current schema file needs this (136 featuresourceentries checked, none are URLs). Documented as an explicit decision indocs/platforms/README.md, revisit if a feature genuinely needs a URL source.note/statusit documents goes stale) is not detectable by CI — only syntactic drift (renamed/removed symbols) is, and only for the file trees now covered by ci(platform-schema): expand conformance workflow path triggers to cover adapter source code #1337's trigger paths. Documented as a known limitation; no dedicated owner audits for it.Acceptance Criteria
platform-schema-conformance.ymltriggers on changes tocrates/openab-gateway/src/**,crates/openab-core/src/**, andsrc/main.rs(all current schema code-ref targets, verified by extracting every.rspath cited across all 8 schema files), plus a weeklyschedulebackstop paired withworkflow_dispatchfor manual testing (matches the other 10 schedule-triggered workflows in this repo)platform-schema-conformance.ymlusescheckout@v6, has a Rust build cache, and a concurrency group keyed by bothgithub.event_nameandgithub.ref(apush-to-main run and the weeklyschedulerun both resolvegithub.reftorefs/heads/main; withoutevent_namein the key they'd share a group andcancel-in-progresscould cancel one in favor of the other)check_code_refrejects"file.rs#"(empty symbol) with a clear errorcheck_code_refrejects sources that resolve outside the repo root, including both../-style relative traversal and a bare absolute path (Path::joinreplaces the base entirely for an absolute joined path — a distinct code path from relative traversal)CONTRIBUTING.mdinvites any contributor to update platform schema TOMLs directly, without needing a dedicated ownerdocs/platforms/README.mddocuments both the URL-source decision and the semantic-drift known limitationFollow-ups
At a Glance
Prior Art & Industry Research
Not applicable — CI config, test hardening, and docs changes only; no runtime/architectural/delivery behavior affected.
Proposed Solution
#1337 (
.github/workflows/platform-schema-conformance.yml):crates/openab-gateway/src/**,crates/openab-core/src/**,src/main.rsto bothpull_requestandpushpath triggers — confirmed these cover every.rsfile currently cited by a schema code-ref (extracted allsourcerefs across the 8 platform files)schedule: cron: "0 6 * * 1"(weekly) as a backstop in case a PR touching the source tree somehow bypasses the path triggers, paired withworkflow_dispatch:so it can also be triggered manually (matches every other schedule-triggered workflow in this repo)#1338 (same file):
actions/checkout@v4→@v6(matches the rest of the repo's workflows)Swatinem/rust-cache(same pinned SHA used elsewhere in the repo) scoped tocrates/platform-schema -> targetconcurrency: { group: platform-schema-${{ github.event_name }}-${{ github.ref }}, cancel-in-progress: true }—event_nameis included becausepushandscheduleruns both resolvegithub.reftorefs/heads/main; without it they'd share a group and could cancel each other--lockedwas already present from a prior change; left untouched#1340 (
crates/platform-schema/tests/conformance.rs):check_code_refnow canonicalizes bothrootand the resolvedpathand checksstarts_withbefore proceeding, rejecting any source that escapes the repo root, whether via../..relative traversal or a bare absolute path (Path::joinreplaces the base entirely when the joined path is absolute — a distinct code path from relative traversal)check_code_refnow rejects an empty#symbolexplicitly instead of silently passing (text.contains("")is alwaystruein Rust)check_code_ref_rejects_empty_symbol,check_code_ref_rejects_path_traversal,check_code_ref_rejects_absolute_path#1339 (
CONTRIBUTING.md,docs/platforms/README.md):CONTRIBUTING.md: added a sentence inviting any contributor to update the schema TOML directly when they notice a platform API change or quirk, rather than waiting on a dedicated ownerdocs/platforms/README.md: added a "Known limitation" note under Conformance about semantic vs syntactic driftWhy this approach?
#1337/#1338/#1340 are straightforward hardening matching patterns already used elsewhere in this repo's CI (see
ci.yml,docker-smoke-test.yml) and Rust code (canonicalize-and-check is the standard path-traversal guard). #1337's path list was derived by actually grepping every code-ref in the 8 schema files rather than guessing, to make sure the trigger coverage is complete as of today.#1339: discussed with @sky092879 — two of the issue's three asks were already satisfied by existing docs (
CONTRIBUTING.md:202, README's "Architecture: TOML vs Markdown" section). For the remaining ask, a formal per-platform ownership table was judged to add coordination overhead disproportionate to the current team size, with no clear enforcement mechanism. The underlying concern (stale docs) splits into syntactic drift — now handled by #1337's expanded triggers — and semantic drift, which has no automated solution and is now an explicit, revisit-if-needed limitation instead of an unaddressed gap.Alternatives Considered
feature_sources_exist_in_treeto skip URL sources (issue fix(platform-schema): harden check_code_ref — reject empty symbols and path traversal #1340's third sub-point): considered, rejected for now. No current feature source is a URL (checked all 8 schema files, 136 entries), so this would be a preemptive change with no current benefit, and could let feature sources silently drift to unverifiable doc links instead of code. Documented as an explicit decision indocs/platforms/README.md; revisit if a feature genuinely needs a URL source.Validation
Rust changes:
cargo checkpasses — not verified in this environment (no Rust toolchain available); please verify in CIcargo testpasses (including new tests) — not verified in this environment; please verify in CIcargo clippyclean — not verified in this environment; please verify in CICI/workflow changes:
actions/YAML linter available in this environment)Docs-only changes:
All PRs:
check_code_ref's new logic against the empty-symbol, relative-traversal, and absolute-path cases by hand, and cross-checked ci(platform-schema): expand conformance workflow path triggers to cover adapter source code #1337's new trigger paths against every code-ref actually cited in the 8 schema files, sincecargo testcould not be run locally.concurrencygroup collision betweenpushandschedule, the missingworkflow_dispatch, and the missing absolute-path test case.