Pull request advice
magus can leave one comment on a pull request describing what your workspace's own build graph noticed about the change. Each finding is a section of that single comment, it rewrites itself on every push, and a section disappears the moment its finding stops being true.
None of it blocks a merge. The checks do that.
The same advisors run at the keyboard (magus diff --impact) before you push. See
Before the push.
Turning it off
Every advisor is an input, and every input defaults to true. Set the one you do
not want to false:
- uses: egladman/magus/.github/actions/advice@v0.4.0
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
with:
doctor: 'false'
blast-radius: 'false'
The token is never an input: the step sets GITHUB_TOKEN from a secret in its env,
and the action fails when it is missing.
Nor is the pull request. The action reads it from the event that started the workflow,
so run the step on pull_request (or pull_request_review, for merge-queue); an event
carrying no pull request, such as a push, leaves every advisor silent. A switch reads
'true' or 'false', and anything else fails the step. An advisor that fails is
reported, the rest still run, and the step fails at the end.
To stop the comment entirely, drop the step. If you would rather keep the step and
switch it off without editing the workflow, the job in magus's own ci.yaml is
guarded by a repository variable, which you can copy:
if: github.event_name == 'pull_request' && vars.MAGUS_PR_COMMENTS != 'false'
Set MAGUS_PR_COMMENTS to false under Settings, Variables, and the job stops
running without a commit.
Silencing one finding on one pull request
Turning an input off is a decision about the repository. Silencing is usually a decision about the change in front of you: you looked, it does not apply here, and you want it to stop without committing that opinion to everyone forever.
Label the pull request:
| label | effect |
|---|---|
advice: silence |
mutes every advisor on this pull request |
advice: silence unclaimed |
mutes one, by its input name |
A silenced advisor retracts its section rather than freezing the last thing it said, so the comment never shows a finding nobody is still checking. Remove the label and it comes back on the next push.
The label is visible on the pull request, which is the point: the next reader can see what was muted and by whom.
Before the push: the same advisors, locally
The advisors are not pull-request-only. magus diff --impact runs the read-only ones
against your working tree, in process, before you push:
magus diff --impact
Same scripts, same graph, same wording: the difference is where the answers go. In CI
they are composed into one comment; locally they are a section of the --impact report,
where acting on a finding still costs one edit rather than a review round trip.
Four things differ, all deliberate:
Labels do not apply. advice: silence is a fact about a pull request, and a local run
has none. Nothing is silenced at the keyboard, on purpose: you asked for the report, so
you want everything in it. Silencing is a decision to record where the next reader can
see it, which is the pull request.
The base is whatever your clone has. A local run never fetches. magus diff is a
read-only report, it may run offline, and under --watch it re-fires on every save,
none of which may write refs/remotes/. So it compares against the origin/<base> in
your clone as it stands, and the report says how old that is:
BASE: origin/main, tip 6 days old - a local run stays off the network, so anything
merged since is outside what the advisors saw; `git fetch origin main` brings it forward
This is the one place local and CI legitimately disagree. CI fetches the base before it
compares, so a finding that appears in the comment and not on your machine usually means
your origin/main is behind, not that the advisor changed its mind. Fetch and rerun
before you go looking for a bug.
Uncommitted edits count. CI diffs the pull request's commits, base...head. A local
run diffs the working tree against the merge base, so the change it describes is the one
you are about to commit rather than the one you already did. Committed work is still
included: the merge base is the same starting point either way. Untracked files are
outside both: git diff reports tracked paths, so a brand new file no project claims is
invisible until you git add it.
first-contribution does not run. It asks the forge whether the pull request's author
has merged anything before, and a working tree has no author. Neither does merge-queue,
for the same reason: it reads the pull request's review state and labels. Nor does
trusted-script-moved, which asks the forge for the pull request's renamed and removed
files.
What each advisor says
| input | it comments when |
|---|---|
merge-conflicts |
the pull request conflicts with its base in files magus generates, which a merge driver cannot settle on the server |
unclaimed |
changed files belong to no project, so no target reads them and the checks say nothing about them |
target-outputs |
a new target declares no outputs and edits no file in place, which means it never replays from cache |
skip-cache |
a target opts out of the cache, quoting the reason magus requires for it |
blast-radius |
the change reaches a large share of the workspace, with the chain that pulled each project in |
doctor |
magus doctor reports a failing check; run it locally for the advice tier and its detail |
version-floor |
the pull request raises required_version, which every contributor must act on |
trusted-script-moved |
the change renames or removes a script a job runs from a default-branch checkout, which breaks that job on the pull request or after the merge |
conformance |
a new target, or a symbol the change adds, renames or re-signs, departs from what the rest of the workspace does with the same work or declaration |
missing-target |
the change adds a project, or drops a target, leaving it short of one its kind overwhelmingly has |
exported-api |
the change touches symbols reachable outside the project that defines them, and with a baseline, what it did to each and the smallest bump that proves |
first-contribution |
the author has no merged pull request here yet |
merge-queue |
off by default; the pull request is approved and could join magus queue but has not, naming the label and the methods the provider allows |
merge-queue appears only while the pull request is open, approved, targets
merge-queue-base (the default branch unless set), comes from this repository, and
carries neither auto-merge nor a queue label. It reads the label's prefix and the allowed
merge methods from magus queue describe --provider <merge-queue-provider>, so it names
what the provider accepts, and it retracts once the pull request is queued, merged or
closed. Those states change on labels and reviews, so run it from a workflow triggered
by them, as this repository's queue-advice.yaml does:
- uses: egladman/magus/.github/actions/advice@v0.4.0
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
with:
merge-queue: 'true'
blast-radius takes a fanout-share (default 0.5): the share of the workspace a
change must reach before it says anything. It is a share rather than a count because
five projects is most of a small workspace and a rounding error in a large one.
Its comment links the reached projects to the console's graph view, which draws them in
the browser. console-base (default https://eli.gladman.cc/magus/console/) points the
link at another console. The graph rides in the link's fragment, which the browser never
sends to a server, so the link sends a private workspace's project names nowhere.
trusted-script-moved reads the base's workflows for jobs whose actions/checkout step
sets ref: to the default branch, by name or as github.event.repository.default_branch,
and the paths their run: steps name. Such a job runs the default branch's copy of a
script, so a move the workflow follows skips or fails on the pull request and passes once
it merges; a removal, or a move the workflow does not follow, fails after the merge.
Conformance on code
The symbol half of conformance reads magus diff's checks on each symbol the change
adds, renames or re-signs. Every check derives its norm from the workspace's own symbol
index, needs knowledge.conformance.gates.min_cohort declarations agreeing at
knowledge.conformance.gates.min_share or more (the same two inputs as the target half, 5 and
0.8 until magus.yaml says otherwise), never counts the change's own new names toward a
norm, and states a fact with its counts rather than a rule. The action's
conformance-min-cohort and conformance-min-share inputs override those two gates for one
run. The checks:
| check | it reports |
|---|---|
naming-affix |
a function or type missing the leading or trailing word pair most declarations of its shape that share a word with it carry |
name-collision |
a name at a project's top level that a target, spell, op, charm or diagnostic already carries |
rename-leftover |
a renamed symbol whose old two-word-or-longer name still appears in the files that reference it |
param-order |
a function taking two parameters in the reverse of the order most functions in its scope that take both do |
conformance-threshold |
a symbol past the value knowledge.conformance enforces, or past the base's because the change raised it or listed the symbol as known |
call-sequence |
a function calling the same workspace functions in the same order as another, over a run longer than the call-sequence value for its population: share the sequence in one function, or record a known entry with its reason |
sibling-sprawl |
a method or function joining siblings whose names differ in one complementary word (RecordCacheHit, RecordCacheMiss); it weighs shared parameters, three types carrying the family, and a type past its type-methods value |
shared-loop |
a function shared past shared-body, caller-spread or shared-reach that one caller's branch or option bends, or a re-signed one that gains such a knob: give each caller a copy with one line saying why, or record a mode-knobs known entry |
import-alias |
an aliased import of a package past alias-share or name-clash, or a new package taking an existing one's name: rename the package, or record a known entry with its reason |
The checks read what every language's index holds (names, kinds, scopes, references) and,
where a language reports it, the declaration's shape: param-order needs a language whose
index renders parameter names (Go and TypeScript today). conformance-threshold needs
knowledge.conformance.ratchet to name a committed record; without one it reports nothing. It
judges against the committed value, else the configured anchor, never a percentile measured
today.
call-sequence and sibling-sprawl are on by default and stay advice. call-sequence reads
the index's calls and the lines they sit on, so it needs a language whose index records calls.
It drops a wrapper (a one-statement body three or more functions call) from every sequence,
and leaves out two shapes that share callees by design: a builder calling mostly one type's
methods, and two Go command verbs taking a []string argument list. Its value is
knowledge.conformance.checks.call-sequence, 7 for source and main until the ratchet commits
a tighter one. sibling-sprawl needs a family of three, or two that three types carry, with
two members the change did not add; a lone family clears the bar only with one of its three
weights. Folding the family into one member that takes the word as a value is the usual
remedy.
shared-loop is the mirror of a clone, also advice: a function whose callers do not share its
knobs. A knob is a parameter the body switches on or tests bare, or, on a function three or
more packages call, an options field or With function only one of them uses; a decision
table, a body that is one switch, has none. It fires on a function with a knob whose body is
past shared-body and whose callers are past caller-spread import hops apart (the function's
own package removed) or more than shared-reach packages, and on a re-signed function two
packages or files call that branches on a parameter. The index holds no base signature, so a
re-signed function's every knob is named, whether or not the change added it. Keep a shared
function only when its callers share failure semantics and policy knobs, so a bug in one is a
bug in the other.
import-alias reads an import that must be renamed as advice that a package's name is
wrong, and stays advice. alias-share is the percent of a package's importers that alias it,
leaving out an alias equal to its name and an importer inside a package of that name;
name-clash is the percent of a package's importers that also import another package of its
name, blamed on the package with fewer importers. Both judge only packages with
min_subject importers (5 by default), never a generated or excluded one, and read Go's
package clauses, so another language contributes nothing. The finding names the package's
spellings (one: name the package after it; several: pick one) or the clash and its share,
and every importer.
A directory the change creates gets three more checks, read from magus diff's layout on
the first changed file in it. A directory is new when every file the index places under it
is one the change adds, so an existing directory is never reported. Its norm is the
directories of the same language under the same parent that the change did not create,
behind the same two gates, knowledge.conformance.gates:
| check | it reports |
|---|---|
package-files |
a new directory holding one source file where most beside it hold more |
package-tests |
a new directory holding no test file where most beside it hold one |
package-name |
a new directory with a digit in its name where most beside it have none |
Each finding states the directory's own counts (source files, test files, and the directories
whose files reference what it defines), how many of its siblings follow the norm, and the two
nearest that do. A test file is recognized by its language's own naming (_test.go,
.test.ts, .spec.ts, test_*.py), and a directory mixing two languages is left alone.
These are facts about a boundary the author chose, so they stay advice.
Before the checks run, magus diff brings every symbol index of every project the change
touched up to date through its own op (scip, or the op its indexer declares, such as scip-buzz), so a
current index replays and a stale one rebuilds only itself. When it cannot (the indexer is missing or fails, or cache
writes are off so nothing vouches for the rebuilt index), the review carries
MGS7003 in place of findings, and this section
says so and fails its step. It never reads as a change with nothing to report. The job running
the advisors therefore needs cache writes on (MAGUS_CACHE_WRITE_ENABLED: 'true'); without a
signing key that still publishes nothing to a shared cache.
A baseline sharpens this half as it sharpens exported-api. With one, what the change
adds, renames and re-signs is read from the two indexes. Without one it is read from the
change's own patch: a symbol is new when its definition line is an added line that no
removed line in the patch names, and a re-signed symbol is not compared at all. When the
workflow's baseline step fails, the section says so under its findings rather than quietly
reporting less.
The bump is a floor
exported-api takes a baseline: a magus graph export --symbols -o json of the revision
the pull request started from. With one, it compares every changed symbol against its base
and reports what the change did to it (added, removed, re-signed, or changed in the body)
and the semver bump that evidence proves.
The bump is a LOWER bound and never a ceiling. A removed public symbol proves a major, an added one proves a minor, and nothing a graph holds can prove a change is small: behavior moves under an unchanged signature. So raise it freely and never lower it.
A changed signature is reported apart from the floor, as the likely bump. Signatures are compared as the indexer rendered them, which is what keeps this language-agnostic and is also the one place it can be wrong: renaming a parameter, widening a type, or changing a constant's value all move the rendered text without breaking a consumer.
Index both sides in one checkout. Two environments render one declaration differently
(a missing node_modules turns a TypeScript parameter into any) and every difference
would read as a changed signature.
git worktree add /tmp/base "$(git merge-base HEAD origin/main)"
magus --root /tmp/base graph build
magus --root /tmp/base graph export --symbols -o json --tee /tmp/base.json -q > /dev/null
magus diff --baseline /tmp/base.json
Why a single comment
A comment per finding turns a pull request into a wall of bot noise, and a thread that only grows is one people mute. One comment with a section per advisor means one notification, one place to look, and a body that tells you what is true right now rather than what was true three pushes ago.
Each section is collapsed, and its summary line carries the count, so the comment stays a table of contents until you open something.
When it says nothing
A pull request with no findings gets no comment at all. The first advisor with something to say opens the comment, so a clean change carries no trace that magus looked. There is no "nothing found" placeholder: open one five times for nothing and you stop opening it, including the time it has a finding. Your checks list already reports that the run happened.
Once you settle the last finding, magus rewrites the comment to say so and leaves it in place. That keeps the thread and its replies, and it tells whoever reads the merged pull request that magus raised something and that someone handled it.
Every advisor answers one question: what did this pull request do? A finding that
holds on the base branch is backlog, and you work through backlog with magus doctor at
the keyboard.
What it costs
The advisors run after your checks and never gate them, and none of them writes to the
pull request: the action needs pull-requests: write for its comment and nothing more.
Regenerated files reach the base branch through magus queue,
which settles generated-file conflicts at merge time in a job that runs no pull request
code.
See also
- Git integration: what the
merge-conflictsadvisor is telling you to do, and why settling a stack takes merges rather than rebases. - CI integration: wiring magus into a pipeline.
- Cache model: why a target without declared outputs never replays.
- Affected: what "reaches" means, and how magus computes it.