Repository navigation
ci: make the Astryx surface inventory a regenerate-and-diff gate - #3078
Conversation
The check only compared disk to .paths and never ran in CI, so a deleted surface could linger in the Markdown table and a new file could miss both halves. Compare committed artifacts to a fresh generator run and schedule that check when surface files or the inventory move. Fixes apache#3064 Generated-by: Grok
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWhat this solvesThe PR replaces the one-directional Astryx inventory check with a regenerate-and-compare check. It detects:
It adds Source of truthThis extends the existing inventory generator. The generator now accepts an explicit Scope and complexityThe solution is the smallest coherent approach described by the current diff. It reuses the existing generator for both writing and validation. The in-memory render avoids temporary file writes during checks and keeps both artifacts under one source of truth. The CI change adds targeted path detection, a conditional The removed dynamic-import, subprocess, required-surface, Markdown-row, and family-batch checks should remain deleted because generated-artifact comparison covers the resulting files directly. No further deletion is evident without reducing regression coverage. Risks and validationByte-for-byte comparison makes generator ordering, formatting, column, and path changes visible. Contributors must regenerate the committed artifacts after intended inventory changes. The path classifier must include renderer and UI product sources, inventory scripts, inventory artifacts, and inventory documentation. It must exclude runtime-only and unrelated UI test changes as intended. The CI job uses The available evidence confirms the new npm scripts, renderer export, comparison logic, path classification, and conditional CI job in the current diff. Required check results remain unverified without direct CI output. Review-relevant risksNo protected-area effect was identified in the current diff. The changes affect CI configuration, inventory tooling, generated artifacts, and developer-facing npm scripts. The person performing the merge must review the final diff, and a maintainer makes the final determination. WalkthroughThe PR centralizes Astryx inventory rendering, adds byte-for-byte artifact validation, detects relevant changes in CI planning, and runs the check in a conditional GitHub Actions job. ChangesAstryx inventory enforcement
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The PR makes the generated Astryx surface inventory fail CI when it is stale or inconsistent and runs the check for relevant changes; no actionable merge-blocking risk remains beyond normal checks and review. Sequence Diagram(s)sequenceDiagram
participant ChangedFiles
participant ci-test-plan
participant GitHubActions
participant checkAstryxSurfaceInventory
participant renderAstryxSurfaceInventory
ChangedFiles->>ci-test-plan: classify changed paths
ci-test-plan-->>GitHubActions: emit astryx_surface
GitHubActions->>checkAstryxSurfaceInventory: run when astryx_surface is true
checkAstryxSurfaceInventory->>renderAstryxSurfaceInventory: render inventory artifacts
renderAstryxSurfaceInventory-->>checkAstryxSurfaceInventory: return Markdown and paths
checkAstryxSurfaceInventory-->>GitHubActions: pass or report artifact drift
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 02b1bf28-70c3-413c-abc4-b6c63aab633f
📒 Files selected for processing (5)
.github/workflows/ci.ymlpackage.jsonscripts/check-astryx-surface-inventory.mjsscripts/ci-test-plan.mjsscripts/generate-astryx-surface-inventory.mjs
Give the inventory job contents:read and stop persisting checkout credentials. Analyze files from the repoRoot passed to the renderer instead of the module-global checkout path. Generated-by: Grok
|
I think the problem statement needs one more step before we add enforcement. The issue demonstrates that the generated inventory can drift, but it does not demonstrate that this drift causes an observable failure: who consumes this inventory, what decision or gate depends on it, and what concrete cost or risk follows when it is stale? “A generated file is inconsistent” is a state; “the check is not wired into CI” is a missing control. Neither is necessarily a product or engineering problem unless the artifact has a clear consumer and correctness requirement. That distinction matters here because
The implementation is a clean solution to “how do we prevent inventory drift,” but the PR/issue should first establish “why is preventing this drift worth permanent CI and maintenance cost?” Without that evidence, this risks enforcing the consistency of an artifact that may not need to exist. |
|
Checked the consumer question against the current tree.
So the issue showed two derived artifacts disagreeing, not an observable product failure. Source files plus the generator are the authority. A regenerate-and-diff job is a control for that derived pair, not a bug fix. If maintainers want a living human audit, one generated Markdown file is enough and Generated-by: Grok |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Fast-path self-merge per AGENTS.md: low impact, easy to reverse, does not touch protected areas (Runtime Host execution authority, @maka/eval semantics, public Eval CLI, security, licensing, releases, governance), and passes all required checks. Human contributor yuhan reviewed the final diff and chose the fast path.
3 participants
Summary
The surface inventory is a generated pair of files. The old check only asserted disk →
.paths, did not fail a Markdown row whose file was gone, and was not on any CI path.The check now regenerates both artifacts in memory and compares them to the committed files. A deleted surface, a hand-edited half, or column drift all fail the same way.
npm run astryx:surface-inventory:writeis the fix.CI runs that check on a cheap
astryx_surfacejob when renderer/UI product files or the inventory scripts/artifacts change. Docs-only inventory edits also select it; a runtime-only change does not.Fixes #3064
Verification
npm run astryx:surface-inventory— ok (186 files, 1 exclusion).pathsfails within .paths but not on diskplanTests(['docs/astryx-surface-file-inventory.md'])→astryxSurface: true,code: falseplanTests(['apps/desktop/src/renderer/app-shell.tsx'])→astryxSurface: trueplanTests(['packages/runtime/src/agent-run.ts'])→astryxSurface: falseplanTests(['packages/ui/src/__tests__/foo.test.ts'])→astryxSurface: falseAI use
Select exactly one:
Tool(s) and scope:
Grok authored the implementation and this PR description.
Checklist
Does this PR entail a change in behavior?