diff --git a/packages/cli/src/commands/review/lib/agent-briefs.ts b/packages/cli/src/commands/review/lib/agent-briefs.ts index 16bd58247b9..57900fa2c95 100644 --- a/packages/cli/src/commands/review/lib/agent-briefs.ts +++ b/packages/cli/src/commands/review/lib/agent-briefs.ts @@ -647,6 +647,8 @@ For each finding you were given: **When the defect is mechanically enumerable, sweep the real population — the count is the verdict.** For a claim about a pattern, a predicate, or a parser ("this misclassifies X", "this mishandles shape Y"), do not stop at the one reported instance: run the check over every real instance this repo holds (every workflow step body, every call site, every input the code will actually see) and report the count. "195 of 434 real \`run:\` bodies reach this path" confirms the finding, sizes its severity, and hands the author a number they can re-run rather than argue with — and a count of **zero** is the quoted contradiction that rejects it. Two rules keep a sweep evidence rather than theatre: its oracle must be an **external authority** — the real parser, the real tool, \`bash -n\` — never your own reimplementation of the logic under test, because a mirror shares the blind spots of what it mirrors and mirrored sweeps have manufactured false findings out of their own bugs; and spot-check one hit by reading it before you quote a nonzero count. +**The commonest sweepable thing in a diff is a hardcoded table of another system's namespace** — heap-space names, error codes, MIME types, status codes, locales, a runtime's own enums. It reads as data rather than logic, so it invites being checked by eye against a list you retype; a retyped list is a mirror of the thing under test, which the oracle rule above already rejects, and it is the mirror you are most likely to type correctly and therefore believe. **Parse the literal out of the source** and take the set difference against the authority at runtime — the real enum, the real registry, the real API call. Both directions are findings and they are not the same finding: a name the table has and the authority does not is dead weight; a name the **authority** has and the table does not is an under-count, which is the direction that ships and the one no test written against the table can see, because the table is what those tests enumerate. + **A suggested fix you did not run is a hypothesis; say which one you are giving.** When a finding's fix is cheap to apply, patch it in, re-run the same probe/harness to show it works, then revert — and state that every other number in your report comes from the unmodified PR (the contamination line is what lets a reader trust the rest). A fix too costly to verify is still worth proposing, labeled untested. **A probabilistic failure gets a RATE, not an anecdote.** For a timing/race claim, run N repetitions per arm and report the rates as the verdict; amplify with full CPU load to force the window open (a live case went from 4/11 idle to 5/5 loaded). And attribute honestly: a lower idle rate with no structural change is luck, not a fix. Fake-timer tests hardcode one ordering by construction — they cannot discriminate a race, so a green fake-timer suite is non-evidence here. @@ -671,6 +673,8 @@ It builds the merge base in a sibling worktree and reports \`available\` and \`p A finding an A/B settled carries \`Source: [probe]\` like any other run-produced evidence, with both sides' output quoted. **Do not remove the base tree** — \`cleanup\` sweeps it at the end of the review, and a later finding may need it. +**When the claim is about a version you are not running, fetch that version — the A/B's other axis is not always git.** "This handles the next major", "this survives the runtime's next release", "this parses the format's new field" cannot be settled on the one runtime, dependency or format version the harness happens to have, and a green CI does not settle it either: a matrix is evidence about the versions in the matrix. Install the version the claim names, run the **smallest discriminator** on both — often one expression, not a suite — and quote both outputs. Measured: a heap-space classifier written against the eleven space names Node 22 reports covers them with nothing left over, and silently drops the two more that Node 24 adds. This is cheap where \`base-tree\` is not — a download and one \`-e\`, no dependency install and no build — so keep it to the versions the claim itself names (a claim about a support range names its floor and its newest, which is two runs, not a matrix), and a version you cannot fetch is \`witness: not run — \` like any other unreachable claim. + **When the claim is about what a WORKFLOW does, run the step — do not read the YAML.** A finding against a CI workflow ("this step posts the wrong body", "the sanitizer is bypassed on this path", "this only changed a log line") is a claim about a shell script that happens to live inside YAML, and reading it in place is where workflow review goes wrong quietly: the \`run:\` body is indented inside a block scalar, the \`env:\` that decides its behaviour is spread over three levels (workflow, job, step — nearest wins, and two of them are nowhere near the step), and every \`\${{ … }}\` is a hole the reader fills in from imagination. Lift it out instead: \`\`\`bash @@ -696,6 +700,8 @@ until [ -s /mock.json ]; do sleep 0.1; done # its port is in that rep For anything that is not one of those two wires — the project's own HTTP service, an MCP server, an OAuth endpoint — stand it up yourself and let \`drive\` own the lifecycle. +**When the observable is an AGGREGATE, change the population rather than instrumenting the reader.** A total, a maximum across children, a count over a fleet — a claim about how one of those combines cannot be settled from a single reading, and the obvious repair (add a per-component dump and read that) costs the verdict its standing, because the numbers then come out of a build you edited. Shrink the contributing population instead: read the aggregate with every contributor live, remove exactly one — kill the process, unregister the workspace, drop the feed — and read it again. Both numbers come from unmodified code, and what they mean depends on the combining rule you are testing for: doubled with the population is a sum, flat is not a sum, and reducing the population to a single contributor makes the reading that contributor's own value outright. Only for a sum is the **difference** a contributor's value; under a maximum, removing a non-holder moves nothing and removing the holder exposes the next-largest. Identify what you removed by something the product did not choose for you — a process's own working directory, its port, its registered id — because removing the one you assumed answers a different question than the one asked. + **When the claim is about GITHUB's behaviour, neither tree can settle it — only GitHub can.** A claim like "this encoding renders identically and can never ping", "GitHub strips this tag", "this markdown shape closes the fold" is about the comment pipeline's parser, sanitizer allowlist and notification path, none of which exist in this environment — a local markdown library is a model of GitHub, and judging a sanitizer claim against a model of the authority is exactly the parser-divergence failure under review. Measured live: an \`@\` → \`@\` defusal read as sound in every local trace, and GitHub's real renderer registered the mention and fired the notification. So: - **If the environment variable \`QWEN_REVIEW_SCRATCH_REPO\` is set** (an \`owner/repo\` the user designated for disposable test posts), you may adjudicate on the real renderer: post the payload as an issue comment there — \`gh api repos/$QWEN_REVIEW_SCRATCH_REPO/issues//comments -f body=@\` against an issue you created there for this purpose — read it back with \`-H "Accept: application/vnd.github.html+json"\`, and rule on the returned HTML (and, for mention claims, the timeline events). The observation is the verdict; quote it. This is the ONLY write destination other than \`submit\`'s that any part of this review may touch, it is user-designated, and nothing about the PR under review, its code, or its authors may appear in what you post there — post the minimal payload shape, not the report. @@ -707,7 +713,7 @@ Return, for each finding, one verdict: - **confirmed (low confidence)** — the mechanism is real but the trigger is uncertain (timing, environment, configuration). Say what would confirm it. Carry the severity. - **rejected** — the code does not do what the finding claims (**quote the contradicting code**), or it matches an Exclusion Criterion (one-line reason). -**A confirmed Critical returns its witness.** Alongside the verdict, include a \`witness:\` line quoting the observed output that settled it — the probe's two sides, the A/B's \`BASE:\`/\`PR:\` pair, the extracted step's run, the sweep count — trimmed to the deciding lines. When every run-capability above is genuinely inapplicable and the confirmation rests on the trace alone, write the one line \`witness: not run — \` instead; writing that line is also the moment you notice when the claim was runnable after all. This is mechanical downstream — enforced in code at the findings canonicalization, not merely by the orchestrator's read of its rules: a confirmed Critical returning neither the witness nor the reason line is filed at **low confidence** — terminal-only, never posted — whatever your prose argued, because the evidence a run produced is the one part of a Critical its author can act on without re-deriving the bug. +**A confirmed Critical returns its witness.** Alongside the verdict, include a \`witness:\` line quoting the observed output that settled it — the probe's two sides, the A/B's \`BASE:\`/\`PR:\` pair, the extracted step's run, the sweep count, the two versions' outputs, the table's set difference, the two readings either side of a removal — trimmed to the deciding lines. When every run-capability above is genuinely inapplicable and the confirmation rests on the trace alone, write the one line \`witness: not run — \` instead; writing that line is also the moment you notice when the claim was runnable after all. This is mechanical downstream — enforced in code at the findings canonicalization, not merely by the orchestrator's read of its rules: a confirmed Critical returning neither the witness nor the reason line is filed at **low confidence** — terminal-only, never posted — whatever your prose argued, because the evidence a run produced is the one part of a Critical its author can act on without re-deriving the bug. **Rejecting a Critical carries a higher bar than anything else, and it is one-way.** A rejected Critical is gone — no later stage revisits it, it vanishes from both the pull request and the terminal. To reject one you must **quote the specific code that contradicts the claim**. A passing test, a plausible-looking guard, or "I could not reproduce the reasoning" is not enough — when you cannot quote the contradiction, the floor is \`confirmed (low confidence)\`, never rejection. Downgrading is reversible; a human still sees a low-confidence finding under "Needs Human Review". Rejection is not. diff --git a/packages/core/src/skills/bundled/review/DESIGN.md b/packages/core/src/skills/bundled/review/DESIGN.md index fe7692f92c7..c1e7d89c261 100644 --- a/packages/core/src/skills/bundled/review/DESIGN.md +++ b/packages/core/src/skills/bundled/review/DESIGN.md @@ -459,6 +459,28 @@ Three details of that mechanical half are worth naming, because each was a way t - **A `shell:` declared as `bash` is not the runner's default `bash`.** The default is `bash -e {0}`; declaring `shell: bash` (at any level) makes it `bash --noprofile --norc -eo pipefail {0}`. A pipeline whose middle stage fails aborts under one and not the other, so the emitted header carries `set -eo pipefail` or `set -e` accordingly — a distinction that decides whether the extraction measures the same script the runner ran. - **The header must be inert, line by line.** A `env:` value can be a YAML block scalar; commenting only the entry's first line left its continuation lines in command position, and under the header's own `set -e` the step died in its preamble before its body ran. Four steps in this repo produced exactly that. The test oracle asserts the property directly — the file is the header plus the body verbatim, and every line before the body is a comment or a named directive — rather than filtering the output for lines that look executable, a filter that could not tell the header's `set -e` from one the body legitimately contains. +## Why the version axis is not only git + +`base-tree` answers "and what did it do before", which is the git axis, and for two years that was the only axis this pipeline had. It leaves a whole class of claim unfalsifiable: the code is asserted to handle a version of something it does not ship — the runtime it will be executed by, a dependency's next major, a wire format that will gain a field. Nothing in the diff, the merge base, or the test suite can settle that, because every one of them runs on the version the harness already has. CI does not settle it either, and the way it fails to is the trap: a green matrix reads as "this works" when it only ever said "this works on the versions in the matrix". + +The measurement that made this concrete cost one download. Node 22 reports 11 heap spaces, eight of them old-generation, so an eight-name old-generation set classifies every space that runtime exposes with nothing left over — and would have been reported as complete on that evidence alone. Node 24 reports 13: the two extra names, `shared_trusted_space` and `shared_trusted_large_object_space`, are both old-generation, and are exactly the kind a set written against the runtime in hand does not contain. Whether a given set covers them is a one-line question — `v8.getHeapSpaceStatistics()` on both — and unanswerable by reading. + +So the capability is deliberately small. One other version, named by the claim rather than swept; the smallest discriminator rather than the suite; both outputs quoted. The cost asymmetry is the point — a runtime download and one `-e` against `base-tree`'s install-and-build — and it is also the reason to cap it: a capability this cheap invites a matrix, and a matrix spends the review's budget answering versions nobody asked about. An unfetchable version stays `witness: not run`, exactly like infrastructure the harness lacks, because a claim the harness cannot reach is a fact about the harness. + +## Why the table sweep is called out separately from the impact sweep + +The impact sweep's oracle rule — an external authority, never a reimplementation — was written for parsers and predicates, where the temptation to mirror is obvious enough that naming it is usually enough. The hardcoded enumeration is the same failure wearing a disguise: a table of another system's namespace (heap spaces, error codes, MIME types, status codes, locales) is small, legible, and looks like data rather than logic, so a verifier checks it by reading it against a list they type out — and a hand-typed list is a mirror, with the additional hazard that a table short enough to retype accurately is a table one feels confident about. + +Two properties make the check worth naming rather than leaving to the general rule. First, the literal is already in the diff, so the sweep's population comes free: parse the table out of the source, take the set difference against the authority at runtime, done. Second, the two directions are different findings with different severities, which a "does the table look right" reading collapses. A name the table has and the authority does not is dead weight. A name the **authority** has and the table does not is an under-count — and an under-count is invisible to every test written against the table, because the table is what the test enumerates. That asymmetry is the whole reason the check pays: the failure it catches is the one the diff's own tests cannot. + +## Why an aggregate is verified by removing contributors, not by instrumenting the reader + +A claim about an aggregate — this total is a maximum and not a sum; this field is per-child and not fleet-wide; a child that reports nothing is skipped rather than counted as zero — cannot be settled from the aggregate alone, and the obvious repair is to add a per-component dump. That repair costs the verdict its standing. The numbers then come out of a build the review edited, and the one thing a witness is for is being a run of the code under review rather than a run of the reviewer's version of it. + +Removing a contributor inverts the dependency. The reader is untouched, the population changes, and both readings come from unmodified code. The delta is the removed contributor's own value only where the rule is a sum; the general form is weaker and more useful — how the aggregate responds to a population it did not choose is the combining rule itself, and a population reduced to one contributor reads that contributor outright. It is also usually available — processes can be killed, workspaces unregistered, feeds dropped — and it produces the two numbers a sum-versus-maximum question needs without any per-component surface existing at all. A daemon holding two ACP children answered exactly that: summed RSS moved 193.6 → 377.5 MB across the two while the reported heap peak moved 103.5 → 103.7 MB. That pair excludes a sum — a sum would have moved with the RSS — and no more than that on its own; a fleet-wide maximum, a per-component reading following one child, and the parent's own heap all reproduce it. Which of those it is takes the second removal: take the population down to one contributor and the reading becomes that contributor's own value, or does not. + +The one discipline it needs is identifying what you removed by something the product did not choose for you. Two children of the same daemon are indistinguishable in `ps`; removing "the first one" and reporting the remainder as the second's value answers a question nobody asked. A working directory, a port, a registered id — anything the harness can read back and name — turns the removal into a measurement. + ## Why the round-3 lenses are prose, not detectors Seven lenses joined the briefs from one verification round, and each is a judgment with a crisp trigger rather than a decidable predicate — which is what separates a brief lens from a subcommand here: diff --git a/packages/core/src/skills/bundled/review/SKILL.md b/packages/core/src/skills/bundled/review/SKILL.md index 8e81e415a86..487181dfb59 100644 --- a/packages/core/src/skills/bundled/review/SKILL.md +++ b/packages/core/src/skills/bundled/review/SKILL.md @@ -602,12 +602,18 @@ The brief also carries the **render-adjudication capability**: when the user has The brief also carries the **A/B capability**, which is the probe's counterpart for a claim that a probe structurally cannot settle. A probe runs the PR's code and answers "what does it do now"; it cannot answer "and what did it do before". A whole class of finding is exactly that difference — "this changes the output format", "this only adds a field", "cancelled and failed used to be indistinguishable" — and recovering the old behaviour by reading the diff is the step that goes wrong quietly, because the new lines are always present and always look right. So a verifier facing a comparative claim can run `qwen review base-tree`, which builds the merge base in a sibling worktree, and then run the same input on both sides and quote both outputs. Until this existed, `mergeBaseSha` was used for exactly one thing — choosing the diff range — and no step in this pipeline had ever built the code the PR is a change _to_. It costs an install and a build (reused across the review once built), so it is spent per finding rather than per review, and an unavailable base (no merge base, a stale one, a base that will not compile) is a fact about the harness that never becomes a finding against the PR. +The A/B's version axis is git, and it is not the only one. A claim that the code **handles the next version of something it does not ship** — a runtime whose enumeration changes under it, a dependency that removed an API in its next major, a wire format that gained a field — is unfalsifiable on the one runtime the harness happens to be running, and a green CI does not close it either: a matrix is evidence about the versions in the matrix. So a verifier facing a forward-compatibility claim **installs the other version and runs the smallest discriminator on both**, rather than ruling on the claim from a changelog. This is cheap in a way `base-tree` is not — a download and one `-e`, no dependency install and no build — and it is decisive in a way reading is not: a heap-space set written against the eleven names Node 22 reports classifies cleanly there and silently drops the two more Node 24 reports, and nothing in the source says which of the two you are on. Keep it to the versions **the claim itself names**, and quote their outputs side by side as the witness; a version the harness cannot fetch is `witness: not run — ` like any other unreachable claim. Usually that is one other version; a completeness claim over a support range names two — the floor and the newest — which is the bounded exception rather than a licence. Anything past what the claim names is a run the review pays for and a verdict nobody asked about. + The brief also carries **`extract-step`**, which is the A/B's counterpart for a claim about a **workflow**. A `run:` script is a shell program that happens to live inside YAML, and reviewing one in place fails in a way reading normal code does not: the body is indented inside a block scalar, the `env:` that decides its behaviour is spread over three levels — workflow, job, step, nearest wins, and two of them sit nowhere near the step — and every `${{ … }}` is a hole the reader silently fills in. `qwen review extract-step` lifts the script out **verbatim** as an executable and reports what the runner would have supplied around it: the merged three-level `env:` with each key's level named, every `${{ … }}` site listed unevaluated (the stub list — the command refuses to invent values), the resolved `shell` and `working-directory`, and a heuristic list of invoked commands. What to stub and what to feed it stays with the verifier, which is the judgment half; with `base-tree`, the two arms of a workflow A/B become two invocations. A `uses:` step has no `run:` and is refused rather than simulated. -**The witness rule.** The capabilities above exist so a verdict can be something a run produced instead of something a reading concluded, and for a **Critical** that difference is the verdict: a confirmed Critical carries a **witness** — the observed output that settled it, quoted and trimmed to the deciding lines — or one line saying why none could run (`witness: not run — `: the claim needs infrastructure the harness lacks, a timing window no probe can pin, state only production holds). The forms a witness takes are exactly the capabilities' outputs: the probe's flip (both sides), the A/B's two quoted outputs, an extract-step run, the failing build/test text a `[build]`/`[test]` finding already carries, the render read-back, and the **impact sweep** below. A confirmed Critical carrying neither the witness nor the one-line reason is not confirmed at the bar this pipeline posts at: sort it **low confidence** — terminal-only, "Needs Human Review" — whatever the verifier's prose says. The demotion is deliberately mechanical, the same shape as the `— [unverified]` tag — and like that tag it has a machine half, not just this rule: `qwen review findings` (Step 6) demotes any high-confidence `[review]`-source Critical that arrives without the `witness` field and names each demotion on stderr, so a sort you miss here is caught at canonicalization rather than posted. Deterministic sources are exempt there by construction — a `[build]`/`[test]`/`[probe]` finding IS a run's output. This is the double-execute lesson made the default instead of the option (measured; DESIGN.md — The double-execute the probe caught), and it is what maintainer dogfooding measured at scale from the other side: in the review rounds that held up, every posted hard finding quoted executed output, and the one claim written from a reading alone was retracted publicly a round later when its first measurement came back zero (measured; DESIGN.md — The read-only claim retracted in round 2 (PR #8225)). +**The witness rule.** The capabilities above exist so a verdict can be something a run produced instead of something a reading concluded, and for a **Critical** that difference is the verdict: a confirmed Critical carries a **witness** — the observed output that settled it, quoted and trimmed to the deciding lines — or one line saying why none could run (`witness: not run — `: the claim needs infrastructure the harness lacks, a timing window no probe can pin, state only production holds). The forms a witness takes are exactly the capabilities' outputs: the probe's flip (both sides), the A/B's two quoted outputs, an extract-step run, the failing build/test text a `[build]`/`[test]` finding already carries, the render read-back, the **version axis**'s two-version pair (above), and — all below — the **impact sweep**, its **table sweep** specialization, and an **isolation by elimination** pair. A confirmed Critical carrying neither the witness nor the one-line reason is not confirmed at the bar this pipeline posts at: sort it **low confidence** — terminal-only, "Needs Human Review" — whatever the verifier's prose says. The demotion is deliberately mechanical, the same shape as the `— [unverified]` tag — and like that tag it has a machine half, not just this rule: `qwen review findings` (Step 6) demotes any high-confidence `[review]`-source Critical that arrives without the `witness` field and names each demotion on stderr, so a sort you miss here is caught at canonicalization rather than posted. Deterministic sources are exempt there by construction — a `[build]`/`[test]`/`[probe]` finding IS a run's output. This is the double-execute lesson made the default instead of the option (measured; DESIGN.md — The double-execute the probe caught), and it is what maintainer dogfooding measured at scale from the other side: in the review rounds that held up, every posted hard finding quoted executed output, and the one claim written from a reading alone was retracted publicly a round later when its first measurement came back zero (measured; DESIGN.md — The read-only claim retracted in round 2 (PR #8225)). **The impact sweep** is the witness form for a defect that is mechanically enumerable — a pattern misused, a predicate that misclassifies, a parser that mishandles a shape. Instead of confirming the one reported instance, run the check over the repo's **real population** (every workflow step body, every call site, every input the predicate will actually see) and quote the count. "195 of 434 real `run:` bodies reach this path" is at once the confirmation, the severity evidence, and a number the author can re-run rather than argue with — and "0 of 434" is the retraction that keeps a false Critical off the PR. Two guards keep a sweep evidence rather than theatre: its oracle must be an **external authority** — the real parser, the real tool, `bash -n` — never a reimplementation of the logic under test, because a mirror of the implementation shares its blind spots and mirrored sweeps have manufactured false findings twice (measured; DESIGN.md — The mirrored oracle's false positives (PR #8225)); and a nonzero count is spot-checked by reading one hit before it is quoted. +**The table sweep** is that rule aimed at the commonest enumerable a diff contains: a hardcoded table mirroring **another system's namespace** — heap-space names, error codes, MIME types, status codes, locales, a runtime's own enums. Agent 3b flags hand-rolling such a surface when its entrance space is unbounded (the enumeration trap); a bounded namespace is the carve-out that lens names, so most of these tables are legitimate — and a diff that enumerates one leaves something checkable in a single step. **Parse the literal out of the source rather than retyping it**: a retyped table is a mirror of the thing under test, which the oracle rule above already rejects, and it is the mirror most likely to be typed correctly and therefore believed. Then take the set difference against the authority at runtime — the real enum, the real registry, the real API call. Both directions are findings, and they are not the same finding: a name the table has and the authority does not is a dead entry, while a name the **authority** has and the table does not is a silent under-count, which is the direction that ships and the direction no test written against the table can see. A table is only ever complete with respect to the authority you asked, so run it on the versions its claim covers — for a support range, the floor and the newest, which is the version axis's bounded exception above. + +**Isolation by elimination** is the witness form for a claim about an **aggregate** — a summed gauge, a maximum across children, a count over a fleet. The instinct is to add a per-component dump and read that, and the verdict is then a reading of code the review itself wrote. The cheaper move runs the other way: **shrink the contributing population instead of instrumenting the reader**. Take the aggregate with every contributor live, remove exactly one — kill the process, unregister the workspace, drop the feed — and take it again; both numbers come out of unmodified code. Read the pair for the combining rule rather than as a subtraction: doubling with the population is a sum, holding flat is not one, and reducing the population to a single contributor makes the reading that contributor's own value outright. The **difference** is a contributor's value only under a sum — under a maximum, removing a non-holder moves nothing and removing the holder exposes the next-largest. It settles the questions an aggregate cannot answer about itself, which is a larger class than it looks: whether a total is a sum or a maximum (a two-child daemon whose summed RSS moved 193.6 → 377.5 MB while its reported heap peak moved 103.5 → 103.7 MB has answered it), and whether a field is per-component or fleet-wide. It does not settle every question of that family: whether a contributor reporting nothing is skipped or folded in as a zero is invisible under a sum and a maximum alike, and shows only in a figure a zero would move — a count, a denominator, an average. Identify the contributor you remove by something the product did not choose for you — a process's own working directory, its port, its registered id — because removing the one you assumed is how this quietly answers a different question than the one asked. + **After verification:** remove all rejected findings. Separate confirmed findings into two groups: high-confidence and low-confidence, applying the witness rule as you sort — a Critical whose confirmation carries neither witness nor the one-line reason lands in the low-confidence group. The witness rides the finding from here on — into the findings artifact (`witness`, Step 6), the terminal report, and, on a posting run, the inline comment body (Step 7) — because the evidence that settled the verdict is the one part of a finding the author can act on without re-deriving the bug. Low-confidence findings appear **only in terminal output** (under "Needs Human Review") and are **never posted as PR inline comments** — this preserves the "Silence is better than noise" principle for PR interactions. ### Pattern aggregation