fix(tools): point orion-ref-gate's failure hint at an in-repo doc (RIG-3344) - #930
Draft
rigel-mintaka wants to merge 8 commits into
Draft
fix(tools): point orion-ref-gate's failure hint at an in-repo doc (RIG-3344)#930rigel-mintaka wants to merge 8 commits into
rigel-mintaka wants to merge 8 commits into
Conversation
…G-3344)
The gate's violation message told the author to "see skill://private-repo-boundary", which does not exist. Worse, it could not: compass ships its own skill set under `config/skills/` (8 skills), and no boundary skill is among them — so the hint was unresolvable in the repo that emits it, handed to an author at the exact moment a fail-closed gate blocks their PR.
Point it instead at `docs/concepts/self-host-and-managed.md`, which is in this repo, is already the canonical statement of the ban ("Never name or point at the private repo"), and is the doc the gate's own header cites as the rule it enforces. Also inline the actionable part of the guidance — say "the managed service", or describe the core capability directly — so the message stands alone without a lookup.
The test pinned the dead URI, so it moves to asserting the two load-bearing substrings. Verified with a positive control: breaking the cited path takes the suite red (1 fail), restoring it green (23 pass), so the assertion defends the contract rather than merely running.
Spec-impact: none. Refs RIG-3344
Co-authored-by: Matt Wilkinson <matt@rigel.build>
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
|
Compass engineering docs preview: https://compass-obs-rig-3344-orion-r.compass-eng-docs.pages.dev Deployed from |
…G-3344) Review found the hint fix was vulnerable to the defect it repaired: the cited doc path was a bare string literal the test only asserted was *present* in the output, never that the file existed. Deleting `docs/concepts/self-host-and-managed.md` outright left the suite 23-green, so a docs reorg would silently kill the pointer again with every gate passing. Nothing else covers it — markdownlint globs `**/*.md` and cannot read a TS string literal, and the gate's own check only greps for the token. Extracts the path as an exported `REMEDIATION_DOC` const, interpolates it into the failure hint, and asserts the file exists. The path is resolved from `import.meta.url` rather than the cwd, so the assertion holds under any invocation. The test now references the same symbol as the message instead of re-typing the string. Control: repointing the const at a nonexistent path fails exactly the new test (23 pass / 1 fail); restoring returns 24/0. Re-ran the reviewer's own probe in an isolated `git archive` tree — deleting the cited doc with the gate source untouched now goes red where it was previously green. biome, typecheck, and `orion-ref-gate:check` all clean. Spec-impact: none. Refs RIG-3344
…344) Review round 2 found the existence assertion added in the previous commit was invisible to the runner that executes it. The new test's real input set includes `docs/concepts/self-host-and-managed.md`, but the `test` task declared only its TypeScript and lockfile, so moon's content hash could not see the doc and replayed a cached green after it was deleted. Verified end to end in a `git worktree` at the prior head: warm the cache, delete the doc leaving the gate source byte-identical, re-run — moon reported `24 pass / 0 fail` while a direct `bun test` in that same tree reported `23 pass / 1 fail`. The guard added to stop a silent docs move was itself silently skippable, which is the same failure shape relocated one layer out from the assertion to the task graph. Declares the cited doc as an input of the `test` task, scoped to the one file rather than `/docs/**` so unrelated docs edits do not re-run the suite. The sibling `design-ledger-gate` sets the precedent, declaring `/docs/designs/**/*.md` on the task whose subject is the live docs tree. The path now appears in two places, so the `REMEDIATION_DOC` comment names the moon input and says to move both. Counterfactual: with the input declared the task hash tracks the doc — warm run `5a90424d`, cached on re-run, and deleting the doc moves it to `cb969d99`, re-runs, and fails `23 pass / 1 fail`, propagating to `orion-ref-gate:ci`. Restoring the doc returns 24/0. Suite, biome, typecheck, and the gate itself all clean. Spec-impact: none. Refs RIG-3344
…IG-3344) Review round 3 asked whether the previous two findings had a third layer, and they do. The declared test input governs moon's cache, which is what the previous commit proved — but it is not what selects the task in CI. On a pull_request, `tools/ci-matrix` computes its closure with `moon query projects --affected`, which walks the project graph and never consults a cross-tree task input; `.github/workflows/ci.yml` already documents that discriminator for another project. Measured with controls at this head: touching the cited doc yields the closure `['flake-gate','root']` with this project ABSENT, while touching the gate's own source yields `['flake-gate','orion-ref-gate','root']`. Driving the real `generate()` with the measured closure gives a bun leg of `['root:ci']` alone, so the gate is not selected. The task-level query does track the doc (`['check','test']`, against `['check']` for an unrelated doc), so the input is correct and simply never consulted on that event. No code change: the input is right and closing the selection gap would be a CI-architecture change affecting every cross-tree-input gate, which is not this PR's scope. What was wrong was the comments' claim of completeness, so both now scope the guarantee to the cache and name the push/nightly full sweep as the backstop — verified to select this project on both events. Spec-impact: none. Refs RIG-3344
…only PR (RIG-3344) Review round 4 found the previous commit's caveat understated its own blast radius. It scoped the unselected case to a PR that moves or deletes the cited doc, which reads as "worst case the pointer goes stale". The gap is wider: a docs-only PR does not select this project at all, and the `check` scan's own `/**/*` input is cross-tree the same way, so a docs-only PR that ADDS a private-repo reference is not gated on that PR either. Verified by planting a real reference in an unrelated doc: the affected closure came back without this project, the generator selected only `root:ci`, and yet running the scan on that same tree fails loudly with the leak verdict. So the gate's primary function is unselected on precisely the change shape most likely to introduce a leak, and the comment whose whole job is honesty about the uncovered surface was understating it. Widens both comments to state that, names `dependsOn: ['root']` as the one-line option and the over-trigger reason it is declined — the same tradeoff `sql-migration-gate` documents — and points at RIG-3381 for the general answer. Also corrects this file's header, which still described CI as a single `moon run :ci` job; that has not been true since the concern matrix landed, and it now sits directly above an accurate description. The other files carrying that line are left alone. No behaviour change: stripping comments from both files leaves the previous head's bytes identical, the doc remains a resolved input of the test task, and the suite, biome, and the gate are clean. Spec-impact: none. Refs RIG-3344
… (RIG-3344) Review round 5 found the previous commit's caveat still had a wrong noun. It said a "docs-only" PR fails to select this project, which tells a reader a code PR is gated. It is not: the `check` scan's `/**/*` input is cross-tree for every path, so ANY PR that does not touch this project's own tree fails to select it. Measured by planting a real private-repo reference in Go code: the closure came back with six projects and this one absent, the generator selected no target for it, and the scan on that same tree exited 1 naming the offending line. The positive control discriminates — touching this project's own source does select it. Also records why `dependsOn: ['root']` is not taken, which is stronger than the over-trigger cost alone: it would pull this project in for most trees but not for dot-paths such as `.github/**`, which produce an empty closure and are nonetheless scanned — verified by planting a reference in a workflow file. Taking it would read as closing a gap it leaves open, which is the failure this PR has spent its rounds repairing. No behaviour change: stripping comments from both files leaves the previous head's bytes identical. Suite, biome, and the gate are clean. Spec-impact: none. Refs RIG-3344
Review round 6 caught a regression the previous commit introduced. While correcting the radius noun, it also rewrote the `dependsOn: ['root']` rationale from a true reason to a false one, and false twice over: it claimed dot-paths such as `.github/**` produce an empty closure, and that the option therefore would not cover them. Neither holds. The root project's source is `.`, so every path marks it affected and `['flake-gate','root']` is the universal floor — no path produces an empty closure. An A/B in a patched worktree confirms the consequence: with the dependency declared, the gate is selected for every dot-path tried, including workflow files, `.gitignore`, `.envrc`, and `.moon/workspace.yml`; without it, for none of them. So the option does close the PR-time gap. The decline stands on the reason it always had — it over-triggers the gate on every repo-root file change, since moon edges are project-level rather than file-level, which is the tradeoff sql-migration-gate documents declining. The clause now says that and nothing more, and states the mechanism it does rest on. The radius correction the previous commit was for is unaffected and was verified precise: every path inside this project's tree selects it, no path outside does, and there are no inbound graph edges to change that. Spec-impact: none. Refs RIG-3344
…ne (RIG-3344) Review round 7 found the previous commit's fix contradicted the clause beside it. Establishing that root's source is `.` — so every path marks it affected — entails that the edge would run the gate on every PR, but the retained cost phrase still said "every repo-root file change". Both cannot hold. Measured which: with the dependency declared, the gate is selected for paths under go/, apps/, packages/, proto/, docs/, and .github/ — none of which touch a repo-root file. Repo-root-level files are a few dozen of several thousand tracked, so the phrase understated the cost by roughly an order of magnitude. It was accurate where it came from — sql-migration-gate's gap really is two root-level configs — and became false when placed after a clause establishing root's source is the whole repo. States the real cost instead, and keeps the sibling precedent while noting its gap is the narrower one. The decline still stands on that cost: the edge would run a 148ms scan on every PR for a leg that already runs, which is a tradeoff worth naming accurately rather than a reason to take or refuse the line without measuring it. Spec-impact: none. Refs RIG-3344
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The gate's violation message told the author to "see skill://private-repo-boundary", which does not exist. Worse, it could not: compass ships its own skill set under
config/skills/(8 skills), and no boundary skill is among them — so the hint was unresolvable in the repo that emits it, handed to an author at the exact moment a fail-closed gate blocks their PR.Point it instead at
docs/concepts/self-host-and-managed.md, which is in this repo, is already the canonical statement of the ban ("Never name or point at the private repo"), and is the doc the gate's own header cites as the rule it enforces. Also inline the actionable part of the guidance — say "the managed service", or describe the core capability directly — so the message stands alone without a lookup.The test pinned the dead URI, so it moves to asserting the two load-bearing substrings. Verified with a positive control: breaking the cited path takes the suite red (1 fail), restoring it green (23 pass), so the assertion defends the contract rather than merely running.
Spec-impact: none. Refs RIG-3344
Co-authored-by: Matt Wilkinson matt@rigel.build