fix(routing)!: make the v1->v2 destination migration fail safe

Code review of a8ef887c5 + 244b7c419 ("a rule's destination is a rule-set,
and nothing else") found that the change rested on a comment that was not
true. ParseUCIExport dropped dst_domain/dst_ip on the strength of "the
migration is re-run on every load"; model.Migrate() actually runs only from
`shaterd migrate`, i.e. the service init and uci-defaults. The daemon's run
path, the SIGHUP reconcile and the panel's config write never migrate.

So an uncommitted migration (a full /overlay is the documented way that
happens) turned `list dst_domain 'bank.ru'` + `target direct` into a rule
with NO matchers, which IS the spelling of a catch-all: generate points
route.Final at it and the LAST such rule wins. One failed `uci commit` sent
every packet on the router out the plain WAN, silently.

Rule.LegacyDst is the tripwire. It is non-empty exactly when the config
still carries the removed options, and three locks hang off it:
  - ParseUCIExport holds such a rule DISABLED. Chosen over "make IsCatchAll
    false" alone, which only covers matcher-less rules: `dst_domain` plus a
    `src` was never a catch-all, and routing it without its destination
    would still have sent a whole subnet direct.
  - IsCatchAll returns false for it, so it can never own route.Final even
    if something hands its Enabled bit back.
  - ApplyProfileRuleOverrides refuses to enable it (a profile with
    `list enable_rule` would otherwise have defeated the parser).
ValidateRules reports it through the existing warning channel, before the
Enabled gate, so the one message explaining the outage is not suppressed by
the fact that caused it. The init script logs a failed migration to syslog
instead of discarding its exit code and stderr.

The write path had none of this. PUT /api/config decodes a Model straight
from the request body and render.go wrote `enabled` from it, so a panel
save erased the operator's lists (as did the subscription cron, which
re-renders the whole package), and a crafted body with Enabled:true and no
LegacyDst put a live matcher-less rule on disk -- the same whole-router
leak, re-entered from the other side. WriteUCI now reads DISK state and
refuses a rule-changing write over an unmigrated config (409, not 500);
non-rule writers pass and legacyDstOpts carries the options across so cron
preserves them; withDiskLegacyDst takes the field from disk so a fabricated
one can never reach the renderer.

Migration hardening: an entry list that migrates to nothing no longer has
its legacy option deleted (that made "matches nothing" silently become
"matches everything"); a hand-written rule-set whose name collides is no
longer allowed to swallow the entries; delete failures propagate instead of
bumping schema_version past them forever; every error path reverts the
staged uci delta so another process's commit cannot flush a half-migration.

untunnelable.go follows the destination out of the rule: a rule whose
rule-sets are known to match by name is still skipped by the ping/IPTV/VPN
plan, as its v1 form was. D21 documents the AND->OR widening for the
engine's TCP/UDP path; it does not follow that a leak-guard should widen
itself during an upgrade, and with target=direct that meant previously
tunnelled ICMP leaving with the client's real address. Inline rule-sets are
now read from the options, so an engine that has not started yet no longer
costs the operator their ping.

Rule-set vocabulary: `full:`/`suffix:`/`keyword:`/`regexp:` in a text list
fetched by URL were dropped with no diagnostic at all (normaliseListDomain
rejects any token with a colon) -- not "reported as an unknown prefix".
Unifying was rejected: published filter lists are full of colon-bearing
syntax, and a third-party `regexp:` is compiled into the router's matcher
and run per query. The difference stands and is paid for in diagnostics,
per list, on every generate. D21 gains the source/vocabulary table.

Panel: the add form warns about a matcher-less rule exactly as the edit
form does, from one shared predicate; its isCatchAll matches the daemon's
new one; an unmigrated rule reads as held-off rather than merely switched
off. The comment promising a "New list" button that D21 rejected is gone.

go build ./..., go vet ./shater/..., go test ./shater/... (13 packages) and
panel `npm run build` are green. NOT yet verified on hardware.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01H4PcWfrBRyg4eWN58axaGN

docs-shater/DECISIONS.md

@@ -480,6 +480,46 @@ rule-sets are ORed, because `rule_set: [a, b]` matches when either matches. Such
 a rule matches more after the migration than before; it affects only configs that
 used both fields at once.
 
+That AND→OR change is about the ENGINE's TCP/UDP path, and it deliberately does
+**not** extend to the untunnelable-protocol plane (`shater/apply/untunnelable.go`,
+the ping / IPTV / VPN-passthrough policy in nftables). There, a v1
+`dst_domain + dst_ip` rule could never claim a packet that carries no domain, so
+the plan skipped it; reading the migrated form as OR would have made the address
+half suddenly decisive, and with `target=direct` that means an upgrade quietly
+sending previously-tunnelled ICMP out with the client's real source address. A
+rule whose rule-sets are known to match by NAME is therefore still skipped by that
+plan, and the skip is reported ("a routing rule matches by name … as well as by
+address"). Split the rule in two if you want the addresses decided there.
+
+### The vocabulary is about ENTRIES YOU TYPE, not about every list body
+The table above is the vocabulary of an **inline** rule-set's `entry` values (and
+of the DNS-filter/device lists, which share the classifier). The other two rule-set
+sources are not other spellings of it:
+
+| source                       | what it is                       | vocabulary |
+|------------------------------|----------------------------------|------------|
+| `inline`                     | entries you type                 | the table above |
+| `url` → `.srs` / `.json`     | a compiled rule-set, engine-owned | the engine's, not ours |
+| `url` → anything else        | a hosts / one-domain-per-line / AdBlock TEXT FILE | **none** — every line is a domain plus its subdomains |
+| `file`                       | a local `.srs` / `.json`          | the engine's, not ours |
+
+**Rejected: run text lists through the entry classifier too.** A published
+AdGuard/OISD list is full of colon-bearing tokens that are ordinary filter syntax
+(`##…:has(…)`, `$domain=`, absolute URLs); classifying them would either mis-import
+them or bury the operator under hundreds of "unrecognised prefix" warnings per
+list. The formats also disagree structurally — a hosts line carries several names,
+so the text parser works per token, while an entry is a whole line. And `regexp:`
+arriving from a third-party URL is a pattern compiled into the router's matcher and
+evaluated per query, which is a very different proposition from one the operator
+typed.
+
+So the difference stands and is paid for in diagnostics instead: a text list
+containing `full:` / `suffix:` / `keyword:` / `regexp:` is reported per list, on
+every generate, naming the entries and pointing at `source=inline` where they work
+(`warnListEntryVocabulary`, `shater/generate/ruleset.go`). The check tests only
+those four markers, never the general `word:` shape, so it fires on a human's
+mistake and stays quiet on published filter syntax.
+
 Consequence: one destination mechanism, one vocabulary, one place a list is
 edited; every list is compiled once and reused. The panel's rule editor drops its
 Domain(s) and IP/CIDR(s) fields; its destination control is a checkbox list of

openwrt/shater-core/files/etc/init.d/shater

@@ -152,7 +152,21 @@ start_service() {
 
 	# Bring the UCI schema forward before the daemon reads it (idempotent;
 	# refuses a newer schema) so an upgraded package never applies a stale config.
-	"$PROG" migrate >/dev/null 2>&1
+	#
+	# THE FAILURE IS LOGGED, NOT SWALLOWED. This is the only place the schema
+	# migration runs at boot (`shaterd run`, the SIGHUP reconcile and the panel's
+	# config write all read UCI directly), so if it fails here it does not get
+	# retried until the next start. And it CAN fail for a mundane reason — a full
+	# /overlay makes `uci commit` fail — after which the config still carries the
+	# schema-v1 `dst_domain`/`dst_ip` options. The daemon holds every rule that
+	# still has them DISABLED and reports it, so nothing is silently misrouted, but
+	# rules the operator wrote are then not in force and the reason has to be
+	# visible somewhere. Hence: log the binary's own stderr, and start anyway —
+	# refusing to start would take the admin panel down with it, and the panel is
+	# the only way to fix the box.
+	local migrate_out
+	migrate_out=$("$PROG" migrate 2>&1) || _slog -p daemon.err \
+		"UCI schema migration FAILED: ${migrate_out:-no output from $PROG migrate}. Starting anyway; routing rules that still carry the removed dst_domain/dst_ip options stay DISABLED until this succeeds. Free space on /overlay and re-run '$PROG migrate', or restart the service."
 
 	procd_open_instance shater
 	# shaterd runs in the FOREGROUND under procd (must never daemonize). `run` is

panel/src/pages/Routing.css

@@ -512,6 +512,8 @@ select.rt-input {
   border-color: var(--accent);
   box-shadow: 0 1px 0 var(--edge) inset, 0 0 0 1px var(--accent-soft);
 }
+/* "no matchers" flag in the plate foot — shared by BOTH rule forms (add and
+ * edit), so the same non-blocking warning reads identically in either. */
 .rt-edit-warn {
   font-family: var(--font-mono);
   font-size: 11.5px;
@@ -563,7 +565,6 @@ select.rt-input {
   color: var(--faint);
 }
 
-
 /* textarea shares the input skin but grows vertically for a list of entries */
 .rt-textarea {
   min-height: 84px;

panel/src/pages/Routing.tsx

@@ -33,6 +33,14 @@ type RRule = Rule & {
   // Minutes east of UTC anchoring the schedule's wall-clock times; captured
   // from the editing browser on save (the router has no tzdata). 0 ⇒ UTC.
   SchedUTCOffset?: number
+  // The daemon's unmigrated-rule tripwire (model.Rule.LegacyDst), read-only here.
+  // Non-empty ⇒ the config STILL carries the schema-v1 `dst_domain`/`dst_ip` that
+  // schema v2 removed, i.e. `shaterd migrate` never ran or could not commit. Each
+  // element is the raw `<option>=<value>` text so the panel can quote what was
+  // found. The daemon holds such a rule disabled; the panel only reports it (it
+  // is never rendered back to UCI, so a config write from here drains it out).
+  // Field name is the Go one: model.Rule has no json tags.
+  LegacyDst?: string[] | null
 }
 
 /**
@@ -212,8 +220,19 @@ function everyLabel(sec: number): string {
 
 /** A rule with no matcher of any kind is the effective catch-all (route Final).
  *  Mirrors model.IsCatchAll on the daemon side — the two must agree or the
- *  "never applies" badge lands on a different row than the apply warning. */
+ *  "never applies" badge lands on a different row than the apply warning.
+ *
+ *  AN UNMIGRATED RULE IS NEVER A CATCH-ALL, and that is the first thing checked
+ *  here, exactly as on the Go side (model/reachability.go). When LegacyDst is
+ *  non-empty the rule's destination is still written in the schema-v1 options
+ *  the parser no longer reads, so its lack of matchers means "the destination is
+ *  unreadable", not "matches everything" — reading it the other way is precisely
+ *  what turned an uncommitted `shaterd migrate` into route Final for the whole
+ *  router. The daemon holds such a rule disabled and reports false here; if this
+ *  copy disagreed, the panel would paint the row "default route · final" while
+ *  the daemon routes nothing through it. */
 function isCatchAll(r: RRule): boolean {
+  if (len(r.LegacyDst) > 0) return false
   return (
     len(r.Src) === 0 &&
     len(r.DstRuleset) === 0 &&
@@ -222,6 +241,36 @@ function isCatchAll(r: RRule): boolean {
   )
 }
 
+/** isCatchAll's twin for a form still being edited: the live fields of either
+ *  rule form with no matcher left in them.
+ *
+ *  Such a rule is not "matches all" in the ordinary sense — the engine emits it
+ *  as route.Final, and among several the LAST one in rule order owns it. So what
+ *  saving one actually does depends on what is last right now, and there are
+ *  three cases:
+ *    - no rules at all, or the last rule is a conditional one → the new rule
+ *      lands last and TAKES the default, silently retargeting every otherwise
+ *      unmatched flow (e.g. the whole LAN to `direct`, past the tunnel);
+ *    - the last rule is already a catch-all → nextOrder() deliberately inserts
+ *      the new one BEFORE it (and bumps the old one up), so the existing default
+ *      keeps route.Final and the new rule is dead on arrival — the daemon
+ *      reports it as shadowed and the row renders as such.
+ *  Both outcomes are worth a warning, and neither form knows which it will be
+ *  (the add form has no rule list), so the shared text says only what is certain:
+ *  the rule has no matchers. It is a legal configuration either way, so neither
+ *  form blocks it — they warn, from this one predicate, so the flag cannot drift
+ *  out of sync between add and edit. */
+function formHasNoMatchers(f: {
+  src: string[]
+  port: string
+  rulesets: string[]
+  proto: string
+}): boolean {
+  return (
+    f.src.length === 0 && f.port.trim() === '' && f.rulesets.length === 0 && f.proto.trim() === ''
+  )
+}
+
 /** Effective routing target for a rule (Target wins; a bare Egress is a target too). */
 function effectiveTarget(r: RRule): string {
   if (r.Target && r.Target.trim()) return r.Target.trim()
@@ -263,8 +312,12 @@ interface TargetGroups {
 // The add form's fields. WHERE traffic is going is a ruleset choice and nothing
 // else — a rule has no inline domain or address list any more, so the old
 // Match-kind picker (rulesets / ip / port) collapsed into a plain Port field
-// beside the ruleset picker. Adding one domain is still one step: the picker can
-// build a list on the spot (RulesetPicker's "New list").
+// beside the ruleset picker. The cost is real and accepted: routing a single
+// domain is no longer done here — you leave for the Rulesets panel, create the
+// list, fill it, and come back to check it. A "create a list from here" shortcut
+// was proposed and rejected (DECISIONS.md D21): a second place to author a list
+// is a second place for its entry semantics and duplicate-name rules to drift,
+// which is the exact thing D21 removed.
 interface AddForm {
   name: string
   src: string[]
@@ -618,6 +671,10 @@ export default function Routing() {
   )
 
   // Insert a new rule just above the catch-all (so a specific rule can actually match).
+  // isCatchAll() is false for an unmigrated rule, which is the right answer here too:
+  // such a rule is held disabled and owns no default route, so there is nothing to
+  // insert ahead of — the new rule simply goes last, where a rule with no matchers
+  // does become the default.
   const nextOrder = useCallback((): { order: number; bumpCatchAll?: { name: string; order: number } } => {
     if (rules.length === 0) return { order: 10 }
     const last = rules[rules.length - 1]
@@ -887,6 +944,15 @@ function RuleRow({
   // matched above" line): those are the claim that made two `default` rules
   // indistinguishable in the first place.
   const dead = shadow !== null
+  // The unmigrated-rule tripwire (see RRule.LegacyDst / model.IsCatchAll): this
+  // rule's destination is still in the removed schema-v1 options, so the daemon
+  // holds it disabled. It is NOT an ordinary disabled rule — nobody switched it
+  // off — so it gets the same "wired but not connected" amber treatment as a
+  // shadowed rule, plus a badge and a line saying what to run. isCatchAll()
+  // already refuses to call it the default, so `final` marks cannot land here.
+  const legacyDst = (rule.LegacyDst ?? []).filter(Boolean)
+  const unmigrated = legacyDst.length > 0
+  const inert = dead || unmigrated
   const isDefault = isCatchAll(rule) && !dead
   const target = effectiveTarget(rule)
   const tone = targetTone(target)
@@ -894,7 +960,7 @@ function RuleRow({
     'rt-rule',
     rule.Enabled ? '' : 'off',
     isDefault ? 'final' : '',
-    dead ? 'dead' : '',
+    inert ? 'dead' : '',
   ]
     .filter(Boolean)
     .join(' ')
@@ -911,7 +977,7 @@ function RuleRow({
         >
           ▲
         </button>
-        <span className={isDefault ? 'rt-ord final' : dead ? 'rt-ord dead' : 'rt-ord'}>
+        <span className={isDefault ? 'rt-ord final' : inert ? 'rt-ord dead' : 'rt-ord'}>
           {isDefault ? '·' : rule.Order}
         </span>
         <button
@@ -929,10 +995,21 @@ function RuleRow({
         <div className="rt-head">
           <span className="rt-name">{rule.Name}</span>
           {isDefault && <span className="rt-badge">default route · final</span>}
+          {unmigrated && <span className="rt-badge dead">held off · not migrated</span>}
           {dead && <span className="rt-badge dead">never applies</span>}
         </div>
         <div className="rt-match">
-          {dead ? (
+          {unmigrated ? (
+            // Why the rule is off and what fixes it. Same voice as the shadow note:
+            // state, cause, one command. The daemon says the same thing through the
+            // apply warnings (model.ValidateRules); this puts it on the row it is about.
+            <span className="rt-dead-note">
+              Destination still written the old way (<span className="mono">{legacyDst.join(', ')}</span>
+              ) — this config was never migrated, so shaterd cannot read where this rule sends
+              traffic and holds it disabled. Run <span className="mono">shaterd migrate</span> on the
+              router to turn those entries into a ruleset, then enable the rule again.
+            </span>
+          ) : dead ? (
             // The badge says it never fires; this line says what beat it and what to
             // do. Visible text, not a tooltip — the operator has to be able to find
             // the other rule, and two rows can carry the same name.
@@ -966,11 +1043,21 @@ function RuleRow({
         >
           Edit
         </button>
+        {/* An unmigrated rule cannot be switched on from here, and the switch says
+            so rather than pretending: the daemon holds it disabled, but a config
+            write from the panel DROPS the unreadable legacy options (render.go
+            emits neither), so enabling it here would save a live rule with no
+            destination left at all — the catch-all this tripwire exists to
+            prevent. `shaterd migrate` clears LegacyDst and the switch comes back. */}
         <Toggle
           pressed={rule.Enabled}
           onChange={() => onToggle(rule.Name)}
-          label={`${rule.Enabled ? 'Disable' : 'Enable'} rule ${rule.Name}`}
-          disabled={frozen}
+          label={
+            unmigrated
+              ? `Rule ${rule.Name} is held disabled until the config is migrated`
+              : `${rule.Enabled ? 'Disable' : 'Enable'} rule ${rule.Name}`
+          }
+          disabled={frozen || unmigrated}
         />
         <button
           type="button"
@@ -1287,6 +1374,14 @@ function AddRule({
   const toggleDay = (d: string) =>
     set('schedDays', form.schedDays.includes(d) ? form.schedDays.filter((x) => x !== d) : [...form.schedDays, d])
 
+  // Same flag as the edit form, from the same predicate: no matcher = this rule
+  // asks to be route.Final. Whether it wins depends on what is last — it takes
+  // the default when there is no catch-all yet (or the last rule is conditional),
+  // and lands dead when there is one, because nextOrder() inserts ahead of it.
+  // Both are worth flagging, so the text states the fact and not the outcome.
+  // Shown, never blocking: a default route is a legal thing to write.
+  const noMatchers = formHasNoMatchers(form)
+
   return (
     <form className="rt-add" onSubmit={onSubmit} aria-label="Add a routing rule">
       <div className="rt-add-hd">
@@ -1375,6 +1470,9 @@ function AddRule({
         <Button type="submit" variant="primary" disabled={busy}>
           {busy ? 'Saving…' : 'Add rule'}
         </Button>
+        {noMatchers && !error && (
+          <span className="rt-edit-warn">no matchers — matches everything</span>
+        )}
         {error && (
           <span className="rt-add-error" role="alert">
             {error}
@@ -1425,9 +1523,15 @@ function RuleEditForm({
   const toggleDay = (d: string) =>
     setSchedDays((cur) => (cur.includes(d) ? cur.filter((x) => x !== d) : [...cur, d]))
 
+  // This rule's destination may still be in the schema-v1 options (RRule.LegacyDst).
+  // The form cannot show or edit them — they are not fields any more — and saving
+  // writes the model back through render.go, which does not emit them. So a save
+  // here silently DISCARDS that destination. Say so before it happens; the fix is
+  // `shaterd migrate`, which converts them into a ruleset this form can check.
+  const legacyDst = (initial.LegacyDst ?? []).filter(Boolean)
+
   // A rule with no matcher of any kind is a catch-all — legal, but worth flagging.
-  const noMatchers =
-    src.length === 0 && port.trim() === '' && rulesets.length === 0 && proto.trim() === ''
+  const noMatchers = formHasNoMatchers({ src, port, rulesets, proto })
 
   const submit = (e: FormEvent) => {
     e.preventDefault()
@@ -1470,6 +1574,12 @@ function RuleEditForm({
           <span className="rt-add-sub">
             Uncheck a list or clear a field to drop that matcher. Save, then Apply.
           </span>
+          {legacyDst.length > 0 && (
+            <span className="rt-edit-warn">
+              not migrated — saving discards its old destination ({legacyDst.join(', ')}); run
+              shaterd migrate first
+            </span>
+          )}
         </div>
 
         <div className="rt-fields">

shater/apply/untunnelable.go

@@ -20,6 +20,7 @@ import (
 	"net/netip"
 	"strings"
 
+	C "github.com/sagernet/sing-box/constant"
 	"github.com/sagernet/sing-box/option"
 	"github.com/sagernet/sing-box/shater/model"
 	"github.com/sagernet/sing-box/shater/netplane"
@@ -30,6 +31,110 @@ import (
 // engine.RuleSetIPCIDRs).
 type ruleSetCIDRs func(tag string) ([]netip.Prefix, bool)
 
+// SCHEMA v2 MOVED A RULE'S DESTINATION OUT OF THE RULE, AND THIS FILE HAD TO FOLLOW.
+//
+// Until D21 a routing rule spelled its destination inline: `dst_domain` became
+// DefaultRule.Domain*, `dst_ip` became DefaultRule.IPCIDR. Both were visible right
+// on the rule, so matchesUntunnelable could read them off it and the ADDRESS half
+// could only ever be resolved through the engine for the rare geosite/geoip case.
+//
+// Since v2 a rule names a `config ruleset` and generate materialises it into
+// Route.RuleSet, so BOTH halves arrive by reference. That broke two things at once,
+// in opposite directions:
+//
+//   - DOMAINS WENT INVISIBLE. A v1 rule `dst_domain=[bank.ru] dst_ip=[10/8]` ANDed
+//     its matchers, so it could not match an ICMP packet (no domain in one) and this
+//     file skipped it. After migration the same rule reads `rule_set: [rule-x,
+//     rule-x-ip]`, whose two sets are ORed — so the address set alone would make the
+//     rule look applicable, and the plan would start emitting an ALLOW (target
+//     direct) or a DENY (target block) for 10/8 that the operator never asked for.
+//     The ALLOW is the dangerous half: it sends previously-tunnelled ICMP out with
+//     the client's real address, i.e. an upgrade silently opening a leak. So a rule
+//     whose rule-sets are KNOWN to carry domain matchers is treated exactly as its
+//     v1 form was: inapplicable to untunnelable traffic. (The OR is deliberate and
+//     documented in D21 for the ENGINE's TCP/UDP path; it does not follow that a
+//     leak-guard should widen itself during an upgrade without a word.)
+//   - ADDRESSES WENT ENGINE-ONLY. `rule-x-ip` is an INLINE rule-set: its prefixes
+//     are sitting right there in the generated options. Asking the engine for them
+//     — and, when the engine is not up, truncating the whole walk and denying
+//     everything — was needless. Inline sets are now read from the options, so an
+//     engine that has not started yet no longer costs the operator their ping.
+//
+// Both come from the same index, built once per plan: what the CONFIG ITSELF can
+// say about each rule-set. Remote/local sets stay the engine's business.
+
+// ruleSetFacts is what the generated options alone reveal about one rule-set.
+//
+// opaque means "not knowable from here" — a local/remote set (only the engine has
+// its contents) or an inline set with a rule shape this code does not model. An
+// opaque set is never used to conclude anything; it falls through to the engine
+// lookup exactly as before.
+type ruleSetFacts struct {
+	opaque    bool
+	hasDomain bool           // carries a matcher only a NAMED destination can satisfy
+	cidrs     []netip.Prefix // addresses, when they are knowable here
+}
+
+// indexRuleSets summarises every rule-set in the generated options by tag.
+func indexRuleSets(sets []option.RuleSet) map[string]ruleSetFacts {
+	out := make(map[string]ruleSetFacts, len(sets))
+	for _, rs := range sets {
+		var f ruleSetFacts
+		// option.RuleSet leaves Type empty for inline in some marshalled forms; the
+		// generator always sets it, and both spellings mean the same thing here.
+		if rs.Type != C.RuleSetTypeInline && rs.Type != "" {
+			f.opaque = true
+			out[rs.Tag] = f
+			continue
+		}
+		for _, hr := range rs.InlineOptions.Rules {
+			if hr.Type != C.RuleTypeDefault && hr.Type != "" {
+				// A logical headless rule, or a shape sing-box adds later. Never guessed
+				// at — the same rule the walk itself follows for a logical route rule.
+				f = ruleSetFacts{opaque: true}
+				break
+			}
+			d := hr.DefaultOptions
+			if headlessNeedsUnavailableMatcher(d) {
+				f.hasDomain = true
+			}
+			f.cidrs = append(f.cidrs, parsePrefixes(d.IPCIDR)...)
+		}
+		out[rs.Tag] = f
+	}
+	return out
+}
+
+// headlessNeedsUnavailableMatcher is matchesUntunnelable's predicate for the
+// HEADLESS rule shape a rule-set carries. It reports whether the rule demands
+// something a packet with no domain, no ports, no sniffed protocol and no process
+// identity cannot supply — in which case that rule-set cannot claim such a packet.
+//
+// The generator only ever emits domain matchers or ip_cidr into an inline routing
+// rule-set, so in practice this is the domain test; the rest is there so a future
+// matcher is refused rather than silently ignored.
+func headlessNeedsUnavailableMatcher(d option.DefaultHeadlessRule) bool {
+	switch {
+	case len(d.Domain) > 0, len(d.DomainSuffix) > 0, len(d.DomainKeyword) > 0,
+		len(d.DomainRegex) > 0, len(d.AdGuardDomain) > 0:
+		return true // no domain in an ICMP/ESP/GRE packet
+	case len(d.Port) > 0, len(d.PortRange) > 0, len(d.SourcePort) > 0, len(d.SourcePortRange) > 0:
+		return true
+	case len(d.Network) > 0, len(d.QueryType) > 0:
+		return true
+	case len(d.ProcessName) > 0, len(d.ProcessPath) > 0, len(d.ProcessPathRegex) > 0,
+		len(d.PackageName) > 0, len(d.PackageNameRegex) > 0:
+		return true
+	case len(d.WIFISSID) > 0, len(d.WIFIBSSID) > 0:
+		return true
+	case d.Invert:
+		// Same reasoning as the route-rule case: inverting flips every conservative
+		// assumption, so the set may only ever withhold, never grant.
+		return true
+	}
+	return false
+}
+
 // buildUntunnelablePlan reduces the resolved routing rules to the first-match
 // walk the nft forward chain can evaluate for a packet that has no domain, no
 // ports and no sniffable protocol.
@@ -57,6 +162,9 @@ func buildUntunnelablePlan(opts option.Options, lookup ruleSetCIDRs) *netplane.U
 		// No routing at all: nothing is provably direct.
 		return plan
 	}
+	// Since schema v2 a rule's destination lives in a rule-set, so the rule-sets
+	// have to be read alongside the rules. See the block above ruleSetFacts.
+	sets := indexRuleSets(opts.Route.RuleSet)
 
 	for _, r := range opts.Route.Rules {
 		if r.Type == "logical" {
@@ -78,7 +186,10 @@ func buildUntunnelablePlan(opts option.Options, lookup ruleSetCIDRs) *netplane.U
 		// and the destination logic never allowed anything. Its `protocol: dns`
 		// matcher makes it inapplicable to a packet with no stream to sniff, which
 		// is the correct and obvious reading once the checks are in this order.
-		if !matchesUntunnelable(d) {
+		if !matchesUntunnelable(d, sets) {
+			if note, ok := namedDestinationNote(d, sets); ok {
+				plan.Warnings = append(plan.Warnings, note)
+			}
 			continue // needs a domain/port/protocol: cannot apply to this traffic
 		}
 
@@ -96,21 +207,28 @@ func buildUntunnelablePlan(opts option.Options, lookup ruleSetCIDRs) *netplane.U
 		mt := netplane.UntunnelableMatch{Allow: verdict}
 
 		// Destination predicate: explicit ip_cidr plus every rule-set's addresses.
+		// An INLINE rule-set is answered straight out of the config — it is fully
+		// present there — so a rule whose lists are all inline no longer depends on
+		// the engine being up. Only local/remote sets still have to be asked for.
 		dst := parsePrefixes(d.IPCIDR)
-		resolvable := true
+		var unresolved string
 		for _, tag := range d.RuleSet {
+			if f, known := sets[tag]; known && !f.opaque {
+				dst = append(dst, f.cidrs...)
+				continue
+			}
 			prefixes, ok := lookup(tag)
 			if !ok {
-				resolvable = false
+				unresolved = tag
 				break
 			}
 			dst = append(dst, prefixes...)
 		}
-		if !resolvable {
+		if unresolved != "" {
 			plan.Warnings = append(plan.Warnings, fmt.Sprintf(
 				"list %q is not loaded yet, so ping/IPTV/VPN passthrough stays blocked for the "+
 					"addresses it covers (it resolves itself once the list downloads)",
-				strings.Join(d.RuleSet, ", ")))
+				unresolved))
 			return plan
 		}
 		hasDstMatcher := len(d.IPCIDR) > 0 || len(d.RuleSet) > 0
@@ -198,7 +316,20 @@ func classifyAction(a option.RuleAction) (isDirect bool, class int) {
 // Every matcher listed here is one the packet cannot satisfy, so a rule carrying
 // any of them is inapplicable rather than universally matching. Matchers ANDed
 // within a rule mean one unsatisfiable matcher makes the whole rule unsatisfiable.
-func matchesUntunnelable(d option.DefaultRule) bool {
+//
+// sets carries the same test for the matchers that are no longer written on the
+// rule: since schema v2 a destination is a rule-set reference, so a rule's domains
+// live in Route.RuleSet rather than in d.Domain*. A rule referencing a set that is
+// KNOWN to match by name is inapplicable here for exactly the reason a d.Domain
+// rule is — the packet has no name to match. Sets whose contents this code cannot
+// see (local/remote) say nothing either way; the address walk below already refuses
+// to conclude anything from them.
+func matchesUntunnelable(d option.DefaultRule, sets map[string]ruleSetFacts) bool {
+	for _, tag := range d.RuleSet {
+		if sets[tag].hasDomain {
+			return false
+		}
+	}
 	switch {
 	case len(d.Domain) > 0, len(d.DomainSuffix) > 0, len(d.DomainKeyword) > 0,
 		len(d.DomainRegex) > 0, len(d.Geosite) > 0:
@@ -229,6 +360,41 @@ func matchesUntunnelable(d option.DefaultRule) bool {
 	return true
 }
 
+// namedDestinationNote explains the one skip an operator can actually be
+// surprised by: a rule that names BOTH a domain list and an address list.
+//
+// A domain-only rule contributes nothing here and always has, so saying so would be
+// noise. A mixed rule is different — its addresses look like something this plan
+// could act on, and it is precisely the shape `shaterd migrate` produces from a v1
+// rule that carried dst_domain and dst_ip together. The rule is skipped so the
+// upgrade cannot quietly change what ping does (see the block above ruleSetFacts);
+// that decision is worth one line in the operator's warning list rather than none.
+//
+// ok=false when there is nothing surprising to report.
+func namedDestinationNote(d option.DefaultRule, sets map[string]ruleSetFacts) (string, bool) {
+	var named []string
+	addressed := len(d.IPCIDR) > 0
+	for _, tag := range d.RuleSet {
+		f, known := sets[tag]
+		if known && f.hasDomain {
+			named = append(named, tag)
+			continue
+		}
+		// An unknown/opaque set may well hold addresses; so may a knowable one.
+		if !known || f.opaque || len(f.cidrs) > 0 {
+			addressed = true
+		}
+	}
+	if len(named) == 0 || !addressed {
+		return "", false
+	}
+	return fmt.Sprintf(
+		"a routing rule matches by name (%s) as well as by address, and ping/IPTV/VPN-passthrough traffic "+
+			"carries no name — so that rule is left out of the ping/IPTV decision entirely and later rules "+
+			"decide those addresses. Split it into a name rule and an address rule if you want the addresses "+
+			"decided here", strings.Join(named, ", ")), true
+}
+
 // parsePrefixes converts CIDR/bare-address strings to prefixes, dropping anything
 // unparseable (it cannot be matched on, so it must not silently widen a set).
 func parsePrefixes(in []string) []netip.Prefix {

shater/apply/untunnelable_test.go

@@ -232,6 +232,139 @@ func TestDomainRuleDoesNotAffectUntunnelable(t *testing.T) {
 	}
 }
 
+// --- schema v2: the destination moved into rule-sets, and so did the analysis ---
+
+// migratedRule reproduces exactly what `shaterd migrate` makes of a v1 rule that
+// carried BOTH lists: `dst_domain=[bank.ru] dst_ip=[203.0.113.0/24]` becomes two
+// inline rule-sets, `rule-x` and `rule-x-ip`, and the rule references both.
+func migratedRule(target string) *model.Model {
+	m := tunnelModel()
+	m.Rulesets = []model.Ruleset{
+		{Name: "rule-x", Type: "domain", Source: "inline", Entries: []string{"full:bank.ru"}},
+		pinnedIPSet("rule-x-ip", "203.0.113.0/24"),
+	}
+	m.Rules = []model.Rule{
+		{Name: "x", Enabled: true, Order: 10,
+			DstRuleset: []string{"rule-x", "rule-x-ip"}, Target: target},
+		{Name: "dflt", Enabled: true, Order: 99, Target: "group:auto"},
+	}
+	return m
+}
+
+// TestMigratedDomainAndIPRuleStaysOutOfTheUntunnelablePlan is the upgrade
+// regression. In v1 the rule ANDed its domain and its addresses, so it could never
+// claim a packet that carries no domain and this plan skipped it. After the
+// migration the same rule reads `rule_set: [rule-x, rule-x-ip]`, and rule_set
+// entries are ORed — so the address set alone made the rule look applicable and the
+// plan started emitting a verdict for 203.0.113.0/24 that nobody asked for.
+//
+// With target=direct that verdict is an ALLOW, i.e. an upgrade quietly sending
+// previously-tunnelled ICMP out with the client's real source address. That is the
+// half that makes this a leak and not just a surprise, so it is checked first.
+func TestMigratedDomainAndIPRuleStaysOutOfTheUntunnelablePlan(t *testing.T) {
+	// Both engine states, because they fail differently: with the engine UP the old
+	// code emitted a live verdict for the addresses, and with it DOWN the same
+	// mistake hid behind the unresolvable-list truncation. Neither may happen.
+	for _, target := range []string{"direct", "block", "group:auto"} {
+		for _, engine := range []string{"up", "down"} {
+			t.Run(target+"/engine-"+engine, func(t *testing.T) {
+				m := migratedRule(target)
+				var loaded map[string][]string
+				if engine == "up" {
+					loaded = map[string][]string{"rs-rule-x": {}, "rs-rule-x-ip": {"203.0.113.0/24"}}
+				}
+				plan := planFor(t, m, loaded)
+
+				if len(plan.Matches) != 0 {
+					t.Fatalf("a rule that matches by NAME as well as by address must contribute no "+
+						"address step (it cannot match a packet that carries no name): %+v", plan.Matches)
+				}
+				if plan.DefaultAllow {
+					t.Errorf("the catch-all routes into the tunnel, so the default must still deny")
+				}
+				fwd := renderPlan(t, m, plan)
+				if strings.Contains(fwd, "203.0.113.0/24") {
+					t.Errorf("the migrated address list must not reach the data plane through the "+
+						"untunnelable policy:\n%s", fwd)
+				}
+			})
+		}
+	}
+}
+
+// TestMigratedDomainAndIPRuleSaysWhyItWasSkipped: skipping is the safe answer, but
+// a silently different ping after an upgrade is the complaint this whole change
+// exists to answer. The mixed rule — the exact shape migration produces — is named.
+func TestMigratedDomainAndIPRuleSaysWhyItWasSkipped(t *testing.T) {
+	plan := planFor(t, migratedRule("direct"), nil)
+	var seen bool
+	for _, w := range plan.Warnings {
+		if strings.Contains(w, "matches by name") && strings.Contains(w, "rs-rule-x") {
+			seen = true
+		}
+	}
+	if !seen {
+		t.Fatalf("the skip must be explained and the list named; warnings=%v", plan.Warnings)
+	}
+}
+
+// TestDomainOnlyRuleSetIsQuiet: a rule whose only destination is a NAME list has
+// always contributed nothing here and is not a surprise, so it must not produce a
+// note. Only the mixed shape is worth a line.
+func TestDomainOnlyRuleSetIsQuiet(t *testing.T) {
+	m := tunnelModel()
+	m.Rulesets = []model.Ruleset{
+		{Name: "names", Type: "domain", Source: "inline", Entries: []string{"full:bank.ru"}},
+	}
+	m.Rules = []model.Rule{
+		{Name: "names", Enabled: true, Order: 10, DstRuleset: []string{"names"}, Target: "group:auto"},
+		{Name: "rest", Enabled: true, Order: 99, Target: "direct"},
+	}
+	plan := planFor(t, m, nil)
+	if !plan.DefaultAllow {
+		t.Errorf("a name-only rule must not withhold the catch-all allow; plan=%+v", plan)
+	}
+	for _, w := range plan.Warnings {
+		if strings.Contains(w, "matches by name") {
+			t.Errorf("a name-only rule is not a surprise and needs no note: %q", w)
+		}
+	}
+}
+
+// TestInlineRuleSetNeedsNoEngine: an inline rule-set's addresses are sitting in the
+// generated config. Asking the engine for them — and, while it is down, truncating
+// the whole walk so ping/IPTV/VPN passthrough dies everywhere — was a conservative
+// answer to a question that did not have to be asked. The verdict must now be the
+// same whether or not the engine is up.
+func TestInlineRuleSetNeedsNoEngine(t *testing.T) {
+	m := tunnelModel()
+	m.Rulesets = []model.Ruleset{pinnedIPSet("pin", "8.8.8.8/32")}
+	m.Rules = []model.Rule{
+		{Name: "pin", Enabled: true, Order: 10, DstRuleset: []string{"pin"}, Target: "group:auto"},
+		{Name: "rest", Enabled: true, Order: 99, Target: "direct"},
+	}
+
+	down := planFor(t, m, nil)                                         // engine not up
+	up := planFor(t, m, map[string][]string{"rs-pin": {"8.8.8.8/32"}}) // engine up
+
+	for name, plan := range map[string]*netplane.UntunnelablePlan{"engine-down": down, "engine-up": up} {
+		if !plan.DefaultAllow {
+			t.Fatalf("%s: the catch-all direct must allow the rest of the internet; plan=%+v", name, plan)
+		}
+		if len(plan.Matches) != 1 || plan.Matches[0].Allow {
+			t.Fatalf("%s: the tunnelled address must produce one DENY step, got %+v", name, plan.Matches)
+		}
+		if got := plan.Matches[0].Dst4; len(got) != 1 || got[0] != "8.8.8.8/32" {
+			t.Fatalf("%s: step destinations = %v, want [8.8.8.8/32]", name, got)
+		}
+	}
+	for _, w := range down.Warnings {
+		if strings.Contains(w, "not loaded yet") {
+			t.Errorf("an inline list is never 'not loaded': %q", w)
+		}
+	}
+}
+
 // TestBlockedRuleDenies: an explicitly blocked destination stays dropped.
 func TestBlockedRuleDenies(t *testing.T) {
 	m := tunnelModel()

shater/generate/dnsfilter.go

@@ -627,6 +627,14 @@ func splitDomainMarker(e string) (marker, value string, ok bool) {
 //	regexp:     NO — warns                    yes — DomainRegex (pattern validated)
 //	geosite:    NO — warns                    NO — warns, entry dropped
 //
+// A THIRD context exists and has NO row here on purpose: the body of a
+// `source=url` plain-text list. That is a hosts/one-domain-per-line FILE parsed by
+// parseDomainList (ruleset.go), not a list of typed entries, and it understands no
+// marker at all — every line is a domain plus its subdomains. An entry written in
+// the vocabulary above is dropped there (a domain cannot contain ":") and reported
+// per list by warnListEntryVocabulary. The long block above parseDomainList states
+// why unifying the two was rejected; do not read this table as covering it.
+//
 // The bare-entry row is now the SAME on both sides, and that uniformity is the
 // point of schema v2 — a routing rule used to read a bare entry as an EXACT host
 // while every list read it as a suffix, a difference nothing in the UI showed.

shater/generate/route_kill_test.go

@@ -8,6 +8,7 @@ import (
 	"strings"
 	"testing"
 
+	C "github.com/sagernet/sing-box/constant"
 	"github.com/sagernet/sing-box/option"
 
 	"github.com/sagernet/sing-box/shater/model"
@@ -45,6 +46,24 @@ func rulesetRuleTarget(rt *option.RouteOptions, name string) (string, bool) {
 	return dr.RuleAction.RouteOptions.Outbound, true
 }
 
+// soleInlineRule returns the single default headless rule an inline rule-set is
+// expected to carry, failing the ASSERTION (rather than panicking on an index) when
+// the set turns out to be remote/local or to hold a different rule shape.
+func soleInlineRule(t *testing.T, rs option.RuleSet) option.DefaultHeadlessRule {
+	t.Helper()
+	if rs.Type != C.RuleSetTypeInline && rs.Type != "" {
+		t.Fatalf("rule-set %q is type %q, not inline — it has no rules in the config to inspect", rs.Tag, rs.Type)
+	}
+	if len(rs.InlineOptions.Rules) != 1 {
+		t.Fatalf("rule-set %q must carry exactly one headless rule, got %d", rs.Tag, len(rs.InlineOptions.Rules))
+	}
+	hr := rs.InlineOptions.Rules[0]
+	if hr.Type != C.RuleTypeDefault && hr.Type != "" {
+		t.Fatalf("rule-set %q carries a %q headless rule, not a default one", rs.Tag, hr.Type)
+	}
+	return hr.DefaultOptions
+}
+
 // TestRuleKillDefaultBlocks: kill="" / "default" is fail-closed — the rule is
 // still emitted, routing its traffic to block. Dropping it would let its traffic
 // fall through to the broader rules below and the default route, and the default
@@ -196,12 +215,29 @@ func TestRulesetMarkerOnlyDomainEntriesDropped(t *testing.T) {
 			[]model.Ruleset{inlineDomainSet("m", entry)},
 			model.Rule{Name: "m", Enabled: true, Order: 10, DstRuleset: []string{"m"}, Target: "node:n1"},
 		)
+		// Only an INLINE rule-set has its rules in the options at all; a remote or
+		// local one (a geosite chip, a compiled url list) carries a URL or a path and
+		// an empty InlineOptions. Indexing Rules[0] unconditionally turned any such
+		// fixture into an index-out-of-range PANIC instead of a failed assertion, so
+		// the shape is checked rather than assumed.
 		for _, rs := range rt.RuleSet {
-			hr := rs.InlineOptions.Rules[0].DefaultOptions
-			for _, list := range [][]string{hr.Domain, hr.DomainSuffix, hr.DomainKeyword, hr.DomainRegex} {
-				for _, v := range list {
-					if strings.TrimSpace(v) == "" {
-						t.Fatalf("%q: emitted an EMPTY matcher token", entry)
+			if rs.Type != C.RuleSetTypeInline && rs.Type != "" {
+				continue
+			}
+			for i, hrule := range rs.InlineOptions.Rules {
+				if hrule.Type != C.RuleTypeDefault && hrule.Type != "" {
+					continue // a logical headless rule has no matcher lists of its own
+				}
+				hr := hrule.DefaultOptions
+				for name, list := range map[string][]string{
+					"domain": hr.Domain, "domain_suffix": hr.DomainSuffix,
+					"domain_keyword": hr.DomainKeyword, "domain_regex": hr.DomainRegex,
+				} {
+					for _, v := range list {
+						if strings.TrimSpace(v) == "" {
+							t.Fatalf("%q: rule-set %q rule %d emitted an EMPTY %s token (an empty domain aborts box.New; an empty keyword matches every host)",
+								entry, rs.Tag, i, name)
+						}
 					}
 				}
 			}
@@ -230,7 +266,7 @@ func TestRulesetMarkerOnlyBesideRealEntriesKeepsTheRest(t *testing.T) {
 	if !ok {
 		t.Fatalf("the usable entries must keep the list alive (warnings %v)", warns)
 	}
-	hr := rs.InlineOptions.Rules[0].DefaultOptions
+	hr := soleInlineRule(t, rs)
 	if len(hr.Domain) != 1 || hr.Domain[0] != "exact.example" {
 		t.Fatalf("domain = %v, want [exact.example]", hr.Domain)
 	}
@@ -262,7 +298,7 @@ func TestRulesetBareEntryIsDomainSuffix(t *testing.T) {
 	if !ok {
 		t.Fatalf("rs-b not emitted; route=%+v", rt)
 	}
-	hr := rs.InlineOptions.Rules[0].DefaultOptions
+	hr := soleInlineRule(t, rs)
 	if len(hr.DomainSuffix) != 1 || hr.DomainSuffix[0] != "example.com" {
 		t.Fatalf("bare entry must become a domain_suffix, got suffix=%v domain=%v", hr.DomainSuffix, hr.Domain)
 	}

shater/generate/ruleset.go

@@ -501,6 +501,47 @@ func ruleSetURLIsEngineNative(rawURL string) (format string, native bool) {
 	}
 }
 
+// TWO WAYS TO WRITE A DESTINATION LIST, AND WHY THEY DO NOT SHARE A VOCABULARY.
+//
+// D21 promises "one destination mechanism, one vocabulary". The vocabulary half of
+// that promise is about the ENTRIES AN OPERATOR TYPES, and those live in exactly
+// one place: an inline `config ruleset` (inlineRulesetRule -> peelDomainRegexes +
+// classifyDomainEntries), where `full:` / `suffix:` / `keyword:` / `regexp:` / a
+// leading dot / a bare name all mean what docs-shater/DECISIONS.md D21 says.
+//
+// A `source=url` list that is not engine-native is NOT another spelling of that.
+// It is a FILE FORMAT — the hosts / plain-domain / AdBlock-ish text third parties
+// publish — and parseDomainList is a parser for that format, not for our entry
+// vocabulary. `source=file` is a third thing again: a compiled .srs or a rule-set
+// .json handed straight to the engine, which never sees shater's entry syntax at
+// all. (An earlier review read this as "the same list written two ways behaves
+// differently"; it is closer to "a typed list and a downloaded file are different
+// artifacts". The diagnostics below exist so an operator never has to guess which
+// one they are looking at.)
+//
+// Unifying them was considered and REJECTED, on three grounds:
+//
+//   - The formats collide. A real AdGuard/OISD list is full of colon-bearing lines
+//     that are not our markers at all (`example.com##.banner:has(...)`, `$domain=`
+//     options, absolute URL rules). Feeding those through the marker classifier
+//     would either mis-import them or, if we reported every `word:`-shaped token as
+//     an unknown prefix, drown the operator in hundreds of warnings per list — a
+//     louder dishonesty than the quiet one it replaces.
+//   - The shapes collide. A hosts line carries SEVERAL names ("127.0.0.1 a.com
+//     b.com"), so this parser works per TOKEN; the entry vocabulary works per LINE
+//     and allows a space after the marker ("keyword: ads"). There is no split rule
+//     that serves both.
+//   - `regexp:` from a URL is regex supplied by a third party, compiled into the
+//     router's matcher and evaluated per query on a 512 MB box. The inline path can
+//     accept it because the operator typed it; a downloaded list is not that.
+//
+// So the difference STAYS, and is paid for in diagnostics instead: a text list that
+// carries our marker vocabulary is reported per list (see listEntryMarker and
+// warnListEntryVocabulary), naming the entries and where they DO work. The check is
+// narrow on purpose — only the four markers D21 defines, never the general `word:`
+// shape — so it fires on an operator's mistake and stays silent on ordinary filter
+// syntax.
+//
 // parseDomainList extracts domains from the formats public blocklists ship in:
 //
 //   - HOSTS         "0.0.0.0 ads.example.com", "127.0.0.1 a.com b.com"
@@ -518,8 +559,12 @@ func ruleSetURLIsEngineNative(rawURL string) (format string, native bool) {
 // de-duplication map is kept — domain.NewMatcher already de-duplicates internally
 // while building the succinct set, so a second map would just double the largest
 // allocation in the pipeline. See listMaxDomains for the measured budget.
-func parseDomainList(content []byte) []string {
+//
+// The second return reports the inline-vocabulary entries seen on the way past, so
+// the caller can say so instead of dropping them without a word.
+func parseDomainList(content []byte) ([]string, listMarkerNote) {
 	out := make([]string, 0, 4096)
+	var note listMarkerNote
 	scanner := bufio.NewScanner(bytes.NewReader(content))
 	// Public lists are one domain per line; 64 KiB is far beyond any real line, and
 	// an over-long line is skipped rather than aborting the parse.
@@ -545,17 +590,123 @@ func parseDomainList(content []byte) []string {
 			// AdBlock-ish "||domain^" -> domain.
 			f = strings.TrimPrefix(f, "||")
 			f = strings.TrimSuffix(f, "^")
+			if _, isMarker := listEntryMarker(f); isMarker {
+				// normaliseListDomain would drop this silently (a domain cannot contain
+				// ":"). Record it so the caller can name it; it is the one class of junk
+				// in a text list that is provably an operator mistake rather than filter
+				// syntax we simply do not import.
+				note.record(f)
+				continue
+			}
 			d, ok := normaliseListDomain(f)
 			if !ok {
 				continue
 			}
 			out = append(out, d)
 			if len(out) >= listMaxDomains {
-				return out
+				return out, note
 			}
 		}
 	}
-	return out
+	return out, note
+}
+
+// listEntryVocabulary is EXACTLY the marker set an INLINE rule-set entry may use
+// (D21). It is deliberately NOT the general `word:` shape unrecognisedDomainPrefix
+// tests for: a published filter list legitimately contains hundreds of colon-
+// bearing tokens, and reporting those would make the diagnostic useless. These
+// four, by contrast, appear in a downloaded text list only when a human wrote them
+// there expecting shater to honour them.
+var listEntryVocabulary = []string{"full:", "suffix:", "keyword:", "regexp:"}
+
+// listEntryMarker reports whether a text-list token is written in the inline entry
+// vocabulary, and which marker it used.
+func listEntryMarker(token string) (string, bool) {
+	lower := strings.ToLower(strings.TrimSpace(token))
+	for _, m := range listEntryVocabulary {
+		if strings.HasPrefix(lower, m) {
+			return m, true
+		}
+	}
+	return "", false
+}
+
+// listMarkerSamples bounds how many offending entries a warning quotes. A list is
+// remote content: it must not be able to write an unbounded amount into our log.
+const listMarkerSamples = 5
+
+// listMarkerNote records the inline-vocabulary entries one plain-text list carried.
+type listMarkerNote struct {
+	Count   int
+	Samples []string
+}
+
+func (n *listMarkerNote) record(entry string) {
+	n.Count++
+	if len(n.Samples) < listMarkerSamples {
+		n.Samples = append(n.Samples, strings.TrimSpace(entry))
+	}
+}
+
+func (n listMarkerNote) empty() bool { return n.Count == 0 }
+
+// listMarkerMemo remembers, per list URL, what the last COMPILATION of that list
+// found. Without it the diagnostic would exist for exactly one reconcile — the one
+// that happened to refresh the artifact — and then vanish for a whole
+// update_interval, which is precisely the "reported to nobody" failure it is meant
+// to fix. Same shape (and same reasoning) as ruleSetProbeCache above; a refresh
+// that finds nothing clears the entry, so fixing the list silences it.
+var (
+	listMarkerMu   sync.Mutex
+	listMarkerMemo = map[string]listMarkerNote{}
+)
+
+func rememberListMarkers(url string, note listMarkerNote) {
+	listMarkerMu.Lock()
+	if note.empty() {
+		delete(listMarkerMemo, url)
+	} else {
+		listMarkerMemo[url] = note
+	}
+	listMarkerMu.Unlock()
+}
+
+func recallListMarkers(url string) listMarkerNote {
+	listMarkerMu.Lock()
+	defer listMarkerMu.Unlock()
+	return listMarkerMemo[url]
+}
+
+// resetListMarkerMemo clears the memo (tests).
+func resetListMarkerMemo() {
+	listMarkerMu.Lock()
+	listMarkerMemo = map[string]listMarkerNote{}
+	listMarkerMu.Unlock()
+}
+
+// warnListEntryVocabulary tells the operator that entries written in the INLINE
+// entry vocabulary were found in a downloaded TEXT list, where they mean nothing.
+// See the parseDomainList block above for why the two vocabularies are separate
+// and why saying so is the whole of the fix.
+func (b *builder) warnListEntryVocabulary(diag, url string) {
+	note := recallListMarkers(url)
+	if note.empty() {
+		return
+	}
+	b.warnf("%s: %q is a plain-text list (a hosts file or one domain per line), but %d of its entries are written in the "+
+		"inline rule-set vocabulary (e.g. %s) — a plain-text list has NO markers, so every line is read as a domain plus its "+
+		"subdomains and anything containing \":\" is dropped, because a domain name cannot contain one. Those entries match NOTHING. "+
+		"Put them in a rule-set with source=inline, which is the one place full:/suffix:/keyword:/regexp: are honoured.",
+		diag, url, note.Count, quoteList(note.Samples))
+}
+
+// quoteList renders sample entries for a diagnostic.
+func quoteList(in []string) string {
+	out := make([]string, 0, len(in))
+	for _, s := range in {
+		out = append(out, fmt.Sprintf("%q", s))
+	}
+	return strings.Join(out, ", ")
 }
 
 // hostsBoilerplate are the names every hosts file carries for its own bookkeeping.
@@ -676,6 +827,11 @@ func (b *builder) compiledListRuleSet(tag, url, updateInterval, diag string) (op
 		}
 	}
 
+	// Reported on EVERY generate, not only on the one that refreshed the artifact:
+	// an entry that matches nothing is exactly as wrong the day after it was
+	// compiled as the moment it was.
+	b.warnListEntryVocabulary(diag, url)
+
 	return option.RuleSet{
 		Type:         C.RuleSetTypeLocal,
 		Tag:          tag,
@@ -717,8 +873,11 @@ func (b *builder) refreshCompiledList(path, url, diag string) error {
 	if err != nil {
 		return err
 	}
-	domains := parseDomainList(body)
+	domains, markers := parseDomainList(body)
 	body = nil // release the source text before the matcher allocates
+	// Remember (or clear) what this compilation saw, so the diagnostic survives the
+	// reconciles that do no I/O at all. See warnListEntryVocabulary.
+	rememberListMarkers(url, markers)
 
 	if len(domains) == 0 {
 		return fmt.Errorf("no usable domains found at %s (fetched %s, but nothing in it parsed as a domain)", url, "the file")
@@ -1232,6 +1391,11 @@ func (b *builder) inlineRulesetRule(rs model.Ruleset) (option.DefaultHeadlessRul
 // that aborts box.New for the WHOLE config, so a bad pattern must degrade to a
 // warning. A BARE `regexp:` compiles fine but matches every host — the same
 // silent match-all hazard as an empty keyword — so it is dropped too.
+//
+// SCOPE: this is the INLINE entry path only, and deliberately so. A `source=url`
+// text list is a hosts/plain-domain FILE, parsed by parseDomainList, which has no
+// marker vocabulary at all — see the block above parseDomainList for why the two
+// are not unified and how an entry written in the wrong one is reported.
 func (b *builder) peelDomainRegexes(diag string, entries []string) (rest, regexes []string) {
 	for _, e := range entries {
 		e = strings.TrimSpace(e)

shater/generate/ruleset_test.go

@@ -1553,7 +1553,7 @@ func TestURLBlocklistNotRefetchedWhileFresh(t *testing.T) {
 // wildcards, IPs and bare labels can never match a domain query, so importing them
 // would be a silent dud (the R9 lesson applied to fetched content).
 func TestParseDomainListRejectsJunk(t *testing.T) {
-	got := parseDomainList([]byte(strings.Join([]string{
+	got, _ := parseDomainList([]byte(strings.Join([]string{
 		"good.example.com",
 		"*.wildcard.example",          // wildcard syntax
 		"/regex/",                     // regex rule
@@ -1584,12 +1584,12 @@ func TestParseDomainListRejectsJunk(t *testing.T) {
 
 // TestParseDomainListHostsEdgeCases covers the messy real-world shapes.
 func TestParseDomainListHostsEdgeCases(t *testing.T) {
-	got := parseDomainList([]byte(stevenBlackSample))
+	got, _ := parseDomainList([]byte(stevenBlackSample))
 	if len(got) != 5 {
 		t.Fatalf("expected 5 domains from the sample, got %d (%v)", len(got), got)
 	}
 	// Unicode is punycoded, matching what actually arrives in a DNS query.
-	uni := parseDomainList([]byte("0.0.0.0 реклама.рф\n"))
+	uni, _ := parseDomainList([]byte("0.0.0.0 реклама.рф\n"))
 	if len(uni) != 1 || !strings.HasPrefix(uni[0], "xn--") {
 		t.Fatalf("a unicode entry must be punycoded, got %v", uni)
 	}

shater/generate/ruleset_vocab_test.go

@@ -0,0 +1,210 @@
+package generate
+
+// The destination-list vocabulary has TWO homes, not one, and the difference is
+// reported rather than silent.
+//
+// docs-shater/DECISIONS.md D21 defines one entry vocabulary — full: / suffix: /
+// keyword: / regexp: / a leading dot / a bare name — and it belongs to the entries
+// an operator TYPES, i.e. an inline `config ruleset`. A `source=url` list that is
+// not engine-native is a hosts/plain-domain FILE, parsed by parseDomainList, which
+// has no markers at all. See the block above parseDomainList (ruleset.go) for why
+// unifying the two was rejected.
+//
+// These tests pin the consequence that makes that acceptable: an entry written in
+// the inline vocabulary inside a text list is NAMED, on every generate, and the
+// check is narrow enough that an ordinary published filter list stays silent.
+
+import (
+	"strings"
+	"testing"
+
+	"github.com/sagernet/sing-box/shater/model"
+)
+
+// urlDomainSet builds a routing `config ruleset` fed from a plain-text URL.
+func urlDomainSet(name, url string) model.Ruleset {
+	return model.Ruleset{Name: name, Type: "domain", Source: "url", URL: url}
+}
+
+// textListModel routes one rule at a url-sourced destination list.
+func textListModel(url string) (sets []model.Ruleset, rule model.Rule) {
+	return []model.Ruleset{urlDomainSet("dest", url)},
+		model.Rule{Name: "dest", Enabled: true, Order: 10, DstRuleset: []string{"dest"}, Target: "node:n1"}
+}
+
+// TestTextListInlineVocabularyIsReported is the regression: `regexp:^ads\.` (and
+// every other inline marker) works in an inline rule-set and matches NOTHING in a
+// plain-text list, because normaliseListDomain drops anything containing ":". The
+// drop is correct — a domain name cannot contain a colon — but it used to happen
+// without a word, so the same string appeared to work in one spelling of "a list of
+// destinations" and to do nothing in the other, with no way to tell which.
+func TestTextListInlineVocabularyIsReported(t *testing.T) {
+	resetListMarkerMemo()
+	t.Cleanup(resetListMarkerMemo)
+	withListFetcher(t, func(string) ([]byte, error) {
+		return []byte(strings.Join([]string{
+			"# a list someone hand-wrote in the inline vocabulary",
+			"0.0.0.0 ads.example.com",
+			`regexp:^ads\.`,
+			"full:exact.example",
+			"keyword:track",
+			"suffix:apex.example",
+			"tracker.example.org",
+		}, "\n")), nil
+	})
+
+	sets, rule := textListModel("https://lists.example/dest.txt")
+	rt, warns := genRulesWithSets(t, sets, rule)
+
+	// The list itself still works — the plain entries are imported as usual.
+	if _, ok := ruleSetByTag(rt, "rs-dest"); !ok {
+		t.Fatalf("the plain entries must still compile into a rule-set; warnings=%v", warns)
+	}
+	if !hasRulesetRule(rt, "dest") {
+		t.Fatalf("the rule referencing the list must still be emitted; warnings=%v", warns)
+	}
+
+	// ...and the four entries that silently vanished are named.
+	if !routeWarnsHave(warns, "inline rule-set vocabulary") {
+		t.Fatalf("marker entries in a text list must be reported, got %v", warns)
+	}
+	// %q-escaped in the message, so match the stable head of the pattern.
+	if !routeWarnsHave(warns, `regexp:^ads`) {
+		t.Fatalf("the warning must quote the offending entry, got %v", warns)
+	}
+	if !routeWarnsHave(warns, "source=inline") {
+		t.Fatalf("the warning must say where those markers DO work, got %v", warns)
+	}
+	if !routeWarnsHave(warns, "4 of its entries") {
+		t.Fatalf("the warning must count every dropped marker entry (4), got %v", warns)
+	}
+}
+
+// TestTextListVocabularyWarningSurvivesAFreshArtifact: the parse that can see the
+// offending entries happens only when the artifact is refreshed, which is once per
+// update_interval. A diagnostic that existed for exactly that one reconcile and
+// then disappeared for a day would be no diagnostic at all, so the finding is
+// remembered per URL and re-reported on every generate.
+func TestTextListVocabularyWarningSurvivesAFreshArtifact(t *testing.T) {
+	resetListMarkerMemo()
+	t.Cleanup(resetListMarkerMemo)
+	var fetches int
+	withListFetcher(t, func(string) ([]byte, error) {
+		fetches++
+		return []byte("keyword:track\ngood.example.com\n"), nil
+	})
+
+	sets, rule := textListModel("https://lists.example/sticky.txt")
+	for pass := 1; pass <= 3; pass++ {
+		_, warns := genRulesWithSets(t, sets, rule)
+		if !routeWarnsHave(warns, "inline rule-set vocabulary") {
+			t.Fatalf("pass %d: the warning must persist while the list does; warnings=%v", pass, warns)
+		}
+	}
+	if fetches != 1 {
+		t.Fatalf("a fresh artifact must not be re-downloaded, fetched %d times", fetches)
+	}
+}
+
+// TestTextListVocabularyWarningClearsWhenTheListIsFixed: the memo is a finding
+// about the list, not a sticky flag. Once a refresh sees a clean list the warning
+// stops, or an operator who fixed the problem would never know they had.
+func TestTextListVocabularyWarningClearsWhenTheListIsFixed(t *testing.T) {
+	resetListMarkerMemo()
+	t.Cleanup(resetListMarkerMemo)
+	body := "keyword:track\ngood.example.com\n"
+	withListFetcher(t, func(string) ([]byte, error) { return []byte(body), nil })
+
+	sets, rule := textListModel("https://lists.example/fixed.txt")
+	if _, warns := genRulesWithSets(t, sets, rule); !routeWarnsHave(warns, "inline rule-set vocabulary") {
+		t.Fatalf("setup: the first pass must report the marker entry; warnings=%v", warns)
+	}
+	body = "good.example.com\nbetter.example.org\n"
+	// The artifact from the first pass is fresh, so force the refresh the operator's
+	// next update_interval would have done anyway.
+	resetListMarkerMemo()
+	listsDirOverride = t.TempDir()
+	if _, warns := genRulesWithSets(t, sets, rule); routeWarnsHave(warns, "inline rule-set vocabulary") {
+		t.Fatalf("a clean list must not keep warning; warnings=%v", warns)
+	}
+}
+
+// TestPublishedFilterListDoesNotTripTheVocabularyWarning is the other half of the
+// trade, and the reason the check tests only the four D21 markers instead of the
+// general `word:` shape unrecognisedDomainPrefix uses. A real AdGuard/OISD list is
+// full of colon-bearing tokens that are ordinary filter syntax; warning about those
+// would put hundreds of lines per list in front of the operator, which is a louder
+// dishonesty than the silence it replaced.
+func TestPublishedFilterListDoesNotTripTheVocabularyWarning(t *testing.T) {
+	resetListMarkerMemo()
+	t.Cleanup(resetListMarkerMemo)
+	withListFetcher(t, func(string) ([]byte, error) {
+		return []byte(strings.Join([]string{
+			"! Title: Example filter list",
+			"! Homepage: https://lists.example/",
+			"||ads.example.com^$third-party",
+			"example.com##.banner:has(> .ad)",
+			"https://tracker.example/pixel.gif",
+			"@@||allowed.example.net^",
+			"0.0.0.0 good.example.net",
+			"fe80::1 ip6-localhost",
+		}, "\n")), nil
+	})
+
+	sets, rule := textListModel("https://lists.example/adguard.txt")
+	rt, warns := genRulesWithSets(t, sets, rule)
+	if _, ok := ruleSetByTag(rt, "rs-dest"); !ok {
+		t.Fatalf("the list must still compile; warnings=%v", warns)
+	}
+	if routeWarnsHave(warns, "inline rule-set vocabulary") {
+		t.Fatalf("ordinary filter syntax must not be reported as a vocabulary mistake: %v", warns)
+	}
+}
+
+// TestParseDomainListReportsOnlyTheInlineMarkers pins the predicate itself, away
+// from the generate machinery: the four markers are recorded, everything else that
+// the parser refuses stays a silent format detail.
+func TestParseDomainListReportsOnlyTheInlineMarkers(t *testing.T) {
+	domains, note := parseDomainList([]byte(strings.Join([]string{
+		"0.0.0.0 kept.example.com",
+		"FULL:Exact.Example", // markers are case-insensitive, like splitDomainMarker
+		"regexp:^ads\\.",
+		"*.wildcard.example", // junk, but not a vocabulary mistake
+		"2001:db8::1",        // colons, but an address — never a marker
+		"nodot",
+	}, "\n")))
+
+	if len(domains) != 1 || domains[0] != "kept.example.com" {
+		t.Fatalf("imported domains = %v, want [kept.example.com]", domains)
+	}
+	if note.Count != 2 {
+		t.Fatalf("marker count = %d, want 2 (FULL: and regexp:); samples=%v", note.Count, note.Samples)
+	}
+	for _, want := range []string{"FULL:Exact.Example", `regexp:^ads\.`} {
+		var seen bool
+		for _, s := range note.Samples {
+			if s == want {
+				seen = true
+			}
+		}
+		if !seen {
+			t.Fatalf("sample %q missing from %v", want, note.Samples)
+		}
+	}
+}
+
+// TestTextListVocabularyWarningIsBounded: the body is remote content, so it must
+// not be able to write an unbounded amount into the operator's warning list.
+func TestTextListVocabularyWarningIsBounded(t *testing.T) {
+	var lines []string
+	for i := 0; i < 500; i++ {
+		lines = append(lines, "keyword:junk")
+	}
+	_, note := parseDomainList([]byte(strings.Join(lines, "\n")))
+	if note.Count != 500 {
+		t.Fatalf("count = %d, want the true total 500", note.Count)
+	}
+	if len(note.Samples) != listMarkerSamples {
+		t.Fatalf("quoted %d entries, want at most %d", len(note.Samples), listMarkerSamples)
+	}
+}

shater/model/migrate.go

@@ -31,6 +31,15 @@ type uciRunner interface {
 	Set(key, val string) error
 	Delete(key string) error
 	Commit(pkg string) error
+	// Revert drops the package's STAGED (uncommitted) changes.
+	//
+	// A migration stages many writes and commits once, so a failure halfway
+	// through leaves a half-migration sitting in /tmp/.uci — and staged changes
+	// are not private to us: the next `uci commit shater` from ANY process (the
+	// panel saving one setting through WriteUCI, an operator at the shell) flushes
+	// them to disk, producing a config that is half v1 and half v2. Every error
+	// path in a migration must therefore revert before returning.
+	Revert(pkg string) error
 	Import(pkg, text string) error
 	// Export returns the package in `uci export` format. ok=false when the package
 	// does not exist (nothing to migrate), which is NOT an error.
@@ -53,6 +62,7 @@ func (execUCI) Get(k string) (string, bool) {
 func (execUCI) Set(k, v string) error { return exec.Command("uci", "set", k+"="+v).Run() }
 func (execUCI) Delete(k string) error { return exec.Command("uci", "-q", "delete", k).Run() }
 func (execUCI) Commit(p string) error { return exec.Command("uci", "commit", p).Run() }
+func (execUCI) Revert(p string) error { return exec.Command("uci", "revert", p).Run() }
 
 func (execUCI) Import(pkg, text string) error {
 	cmd := exec.Command("uci", "import", pkg)
@@ -114,9 +124,23 @@ func ensureGlobals(u uciRunner) {
 func setSchemaVersion(u uciRunner, v int) error {
 	ensureGlobals(u)
 	if err := u.Set("shater.globals.schema_version", strconv.Itoa(v)); err != nil {
-		return err
+		return staged(u, err)
 	}
-	return u.Commit("shater")
+	if err := u.Commit("shater"); err != nil {
+		return staged(u, err)
+	}
+	return nil
+}
+
+// staged discards the package's staged writes and returns err unchanged. It is
+// the one-liner every migration error path goes through: leaving a partial
+// migration staged lets somebody else's `uci commit shater` write it to disk (see
+// uciRunner.Revert). A failing revert cannot be reported on top of the original
+// failure without hiding it, so it is deliberately ignored — err is the one the
+// operator has to act on, and the revert is best-effort cleanup.
+func staged(u uciRunner, err error) error {
+	_ = u.Revert("shater")
+	return err
 }
 
 // Migrate runs pending migrations to CurrentSchemaVersion using the active runner.
@@ -160,7 +184,10 @@ func migrate0to1(u uciRunner) error {
 		}
 		_ = u.Delete("shater.globals.kill")
 	}
-	return u.Commit("shater")
+	if err := u.Commit("shater"); err != nil {
+		return staged(u, err)
+	}
+	return nil
 }
 
 // --- v1 -> v2: a rule's destination is a rule-set, never an inline list ------
@@ -205,7 +232,28 @@ func migrate0to1(u uciRunner) error {
 // IDEMPOTENCE. The legacy options are deleted as the last step per rule, so a
 // second run finds nothing to do. A run interrupted between "create the rule-set"
 // and "delete the option" is also safe: a rule that ALREADY references a rule-set
-// of the expected name reuses it instead of creating `rule-<name>-2`.
+// of the expected name AND WHOSE ENTRIES ARE IN IT reuses it instead of creating
+// `rule-<name>-2` (see ensureMigratedRuleset — the entry check is what tells our
+// own half-finished work apart from an operator's hand-written list that happens
+// to carry the same name).
+//
+// A LEGACY OPTION IS DELETED ONLY WHEN ITS CONTENT HAS A NEW HOME. Deleting is
+// per-list and conditional on that list having produced (or confirmed) a
+// dst_ruleset reference. Unconditional deletion had a hole: a list whose values
+// are all blank (`list dst_domain '  '` — a space survives parseSections' empty
+// check but trims away in migrateDomainEntry) produced no rule-set, and deleting
+// it anyway left the rule with NO matchers at all, i.e. a catch-all that takes
+// over the router's default route. Keeping the option leaves the rule visibly
+// unmigrated instead, which ParseUCIExport holds disabled and ValidateRules
+// reports.
+//
+// ERRORS ABORT AND REVERT. Every write here is staged and committed once at the
+// end, so a failure that returned without reverting would leave a half-migration
+// in /tmp/.uci for the next `uci commit shater` (from the panel, say) to flush.
+// Every error path goes through staged(). Deletes are checked too: a delete that
+// silently failed left a legacy option in the config forever, because migrateWith
+// bumps schema_version to 2 on return and readSchemaVersion never asks for this
+// step again.
 func migrate1to2(u uciRunner) error {
 	text, ok := u.Export("shater")
 	if !ok || strings.TrimSpace(text) == "" {
@@ -216,14 +264,17 @@ func migrate1to2(u uciRunner) error {
 		return fmt.Errorf("read the current config: %w", err)
 	}
 
-	// Every rule-set name already in use, so a generated one can never collide with
-	// a hand-written list (which would make `uci` hold two `config ruleset` blocks
-	// claiming the same name, and the generator drops one as a duplicate tag).
-	taken := map[string]bool{}
+	// Every rule-set name already in use -> its entries, so a generated name can
+	// never collide with a hand-written list (which would make `uci` hold two
+	// `config ruleset` blocks claiming the same name, and the generator drops one as
+	// a duplicate tag). The ENTRIES are carried, not just the name, because
+	// "reuse the existing list" is only correct when that list is the one a previous
+	// run of this migration created.
+	taken := map[string][]string{}
 	for _, s := range secs {
 		if s.Type == "ruleset" {
 			if n := firstNonEmpty(s.opt("name"), s.Name); n != "" {
-				taken[n] = true
+				taken[n] = s.list("entry")
 			}
 		}
 	}
@@ -248,6 +299,7 @@ func migrate1to2(u uciRunner) error {
 		base := rulesetBaseName(firstNonEmpty(s.opt("name"), s.Name), ruleIdx)
 		refs := s.list("dst_ruleset")
 
+		domainsDone, ipsDone := false, false
 		if len(domains) > 0 {
 			entries := make([]string, 0, len(domains))
 			for _, d := range domains {
@@ -255,8 +307,9 @@ func migrate1to2(u uciRunner) error {
 					entries = append(entries, e)
 				}
 			}
-			if err := ensureMigratedRuleset(u, rulePath, base, "domain", entries, refs, taken); err != nil {
-				return err
+			domainsDone, err = ensureMigratedRuleset(u, rulePath, base, "domain", entries, refs, taken)
+			if err != nil {
+				return staged(u, err)
 			}
 		}
 		if len(ips) > 0 {
@@ -266,62 +319,99 @@ func migrate1to2(u uciRunner) error {
 					entries = append(entries, v)
 				}
 			}
-			if err := ensureMigratedRuleset(u, rulePath, base+"-ip", "ipcidr", entries, refs, taken); err != nil {
-				return err
+			ipsDone, err = ensureMigratedRuleset(u, rulePath, base+"-ip", "ipcidr", entries, refs, taken)
+			if err != nil {
+				return staged(u, err)
 			}
 		}
 
-		// Last, so an interrupted run still has the legacy list to redo the work from.
-		_ = u.Delete(rulePath + ".dst_domain")
-		_ = u.Delete(rulePath + ".dst_ip")
+		// Last, so an interrupted run still has the legacy list to redo the work from
+		// — and only for a list whose entries actually reached a rule-set.
+		if domainsDone {
+			if err := u.Delete(rulePath + ".dst_domain"); err != nil {
+				return staged(u, fmt.Errorf("drop the migrated dst_domain of rule %d: %w", ruleIdx, err))
+			}
+		}
+		if ipsDone {
+			if err := u.Delete(rulePath + ".dst_ip"); err != nil {
+				return staged(u, fmt.Errorf("drop the migrated dst_ip of rule %d: %w", ruleIdx, err))
+			}
+		}
 	}
 
-	return u.Commit("shater")
+	if err := u.Commit("shater"); err != nil {
+		return staged(u, err)
+	}
+	return nil
 }
 
 // ensureMigratedRuleset creates the inline `config ruleset` holding entries and
-// points rulePath's dst_ruleset at it, unless the rule already references a
-// rule-set of that name (a re-run after an interrupted migration). want is the
-// preferred name; a collision with an existing list picks want-2, want-3, ...
-// rsType is "domain" or "ipcidr". An entry list that came out empty creates
-// nothing: an empty inline rule-set matches nothing and the generator would skip
-// it, so a dangling reference would be pure noise.
-func ensureMigratedRuleset(u uciRunner, rulePath, want, rsType string, entries, existingRefs []string, taken map[string]bool) error {
+// points rulePath's dst_ruleset at it. want is the preferred name; a collision
+// with an existing list picks want-2, want-3, ... rsType is "domain" or "ipcidr".
+// taken maps every rule-set name already in the config to its entries, and is
+// updated with whatever this call adds.
+//
+// It reports whether the entries now have a home — which is what tells the caller
+// it may delete the legacy option they came from. false with a nil error means
+// "nothing was written and nothing may be deleted": the only such case is an
+// entry list that came out EMPTY (a legacy list of blanks). An empty inline
+// rule-set matches nothing and the generator would skip it, so creating one would
+// be pure noise — but the legacy option must then survive, or the rule is left
+// with no matchers at all and silently becomes the default route.
+//
+// RESUMING AN INTERRUPTED RUN, WITHOUT SWALLOWING A HAND-WRITTEN LIST. A rule
+// that already references a rule-set of the expected name is the signature of a
+// previous run that died between "create the rule-set" and "delete the option" —
+// but it is ALSO what an operator's own `config ruleset name='rule-ads'` plus a
+// rule referencing it looks like. Telling them apart takes one more question:
+// does that rule-set actually CONTAIN these entries? Our own leftover does, by
+// construction. The operator's list (a remote URL list, say) does not, and
+// treating it as "already migrated" threw the legacy entries away with no trace.
+// When the entries are not in it, this falls through to the ordinary collision
+// path and creates want-2, so the rule ends up referencing both lists — the two
+// references are ORed, so nothing the operator wrote stops matching.
+func ensureMigratedRuleset(u uciRunner, rulePath, want, rsType string, entries, existingRefs []string, taken map[string][]string) (bool, error) {
 	if len(entries) == 0 {
-		return nil
+		return false, nil
 	}
-	if taken[want] && containsString(existingRefs, want) {
-		return nil // already migrated (interrupted run); nothing to add
+	if have, ok := taken[want]; ok && containsString(existingRefs, want) && containsAll(have, entries) {
+		return true, nil // already migrated (interrupted run); nothing to add
 	}
 	name := want
-	for i := 2; taken[name]; i++ {
+	for i := 2; ; i++ {
+		if _, clash := taken[name]; !clash {
+			break
+		}
 		name = fmt.Sprintf("%s-%d", want, i)
 	}
-	taken[name] = true
+	taken[name] = entries
 
 	id, err := u.Add("shater", "ruleset")
 	if err != nil {
-		return err
+		return false, err
 	}
 	sec := "shater." + id
 	if err := u.Set(sec+".name", name); err != nil {
-		return err
+		return false, err
 	}
 	if err := u.Set(sec+".type", rsType); err != nil {
-		return err
+		return false, err
 	}
 	if err := u.Set(sec+".source", "inline"); err != nil {
-		return err
+		return false, err
 	}
 	for _, e := range entries {
 		if err := u.AddList(sec+".entry", e); err != nil {
-			return err
+			return false, err
 		}
 	}
 	if containsString(existingRefs, name) {
-		return nil
+		return true, nil
 	}
-	return u.AddList(rulePath+".dst_ruleset", name)
+	if err := u.AddList(rulePath+".dst_ruleset", name); err != nil {
+		return false, err
+	}
+	return true, nil
 }
 
 // rulesetBaseName builds `rule-<name>` from a rule's name, reduced to characters
@@ -381,3 +471,16 @@ func containsString(list []string, want string) bool {
 	}
 	return false
 }
+
+// containsAll reports whether every entry in want is present in have. It answers
+// exactly one question — "is this rule-set the one a previous run of this
+// migration wrote?" — so it is a SUBSET test, not equality: a rule-set that also
+// holds entries the operator added by hand since is still ours to reuse.
+func containsAll(have, want []string) bool {
+	for _, w := range want {
+		if !containsString(have, strings.TrimSpace(w)) {
+			return false
+		}
+	}
+	return true
+}

shater/model/migrate_test.go

@@ -1,7 +1,9 @@
 package model
 
 import (
+	"errors"
 	"fmt"
+	"sort"
 	"strconv"
 	"strings"
 	"testing"
@@ -19,6 +21,7 @@ type fakeUCI struct {
 	secs     []*fakeSection
 	imported string
 	commits  int
+	reverts  int
 	deleted  []string
 	nextID   int
 	// missing makes Export report "no such package", the fresh-install case.
@@ -35,6 +38,38 @@ type fakeSection struct {
 	lKeys []string
 }
 
+// loadSections copies parsed sections into the fake, in a DETERMINISTIC order.
+//
+// uciSection carries its options and lists in maps, and Go randomises map
+// iteration, so feeding them to setOpt/addList in range order made oKeys/lKeys —
+// and therefore the rendered Export — differ run to run. That is not cosmetic
+// here: TestMigrateIdempotent and TestMigrate1to2IsIdempotent compare two export
+// strings, so a random key order turned them into coin flips that fail a few
+// percent of the time and look like a migration bug. Sorting by key restores the
+// determinism the oKeys/lKeys fields were added for.
+func (f *fakeUCI) loadSections(secs []uciSection) {
+	for _, s := range secs {
+		sec := f.newSection(s.Type, s.Name)
+		for _, k := range sortedKeys(s.Options) {
+			sec.setOpt(k, s.Options[k])
+		}
+		for _, k := range sortedKeys(s.Lists) {
+			for _, v := range s.Lists[k] {
+				sec.addList(k, v)
+			}
+		}
+	}
+}
+
+func sortedKeys[V any](m map[string]V) []string {
+	out := make([]string, 0, len(m))
+	for k := range m {
+		out = append(out, k)
+	}
+	sort.Strings(out)
+	return out
+}
+
 func newFakeUCI(export string) *fakeUCI {
 	f := &fakeUCI{}
 	if strings.TrimSpace(export) == "" {
@@ -44,17 +79,7 @@ func newFakeUCI(export string) *fakeUCI {
 	if err != nil {
 		panic("fakeUCI fixture: " + err.Error())
 	}
-	for _, s := range secs {
-		sec := f.newSection(s.Type, s.Name)
-		for k, v := range s.Options {
-			sec.setOpt(k, v)
-		}
-		for k, vs := range s.Lists {
-			for _, v := range vs {
-				sec.addList(k, v)
-			}
-		}
-	}
+	f.loadSections(secs)
 	return f
 }
 
@@ -193,23 +218,20 @@ func (f *fakeUCI) Delete(k string) error {
 
 func (f *fakeUCI) Commit(string) error { f.commits++; return nil }
 
+// Revert is COUNTED, not simulated: this fake applies every write immediately
+// (there is no staging area to roll back), so what a test can assert is that the
+// migration ASKED for a revert on its way out of a failure. That is the property
+// that matters — a real `uci` keeps the staged delta in /tmp/.uci until somebody
+// commits it, and the migration's job is to not leave it there.
+func (f *fakeUCI) Revert(string) error { f.reverts++; return nil }
+
 func (f *fakeUCI) Import(_, text string) error {
 	f.imported = text
 	secs, err := parseSections(text)
 	if err != nil {
 		return err
 	}
-	for _, s := range secs {
-		sec := f.newSection(s.Type, s.Name)
-		for k, v := range s.Options {
-			sec.setOpt(k, v)
-		}
-		for k, vs := range s.Lists {
-			for _, v := range vs {
-				sec.addList(k, v)
-			}
-		}
-	}
+	f.loadSections(secs)
 	return nil
 }
 
@@ -558,6 +580,267 @@ config rule
 	}
 }
 
+// A hand-written rule-set the rule ALREADY references must not swallow the legacy
+// entries. `taken[want] && rule references want` alone reads an operator's own
+// `rule-ads` list as our own half-finished work from an interrupted run, and the
+// legacy `ads.example` was then dropped with nothing said. The entries have to
+// actually BE in that rule-set for it to count as ours.
+func TestMigrate1to2DoesNotFoldIntoAReferencedHandWrittenRuleset(t *testing.T) {
+	f := newFakeUCI(`package shater
+
+config globals 'globals'
+	option schema_version '1'
+
+config ruleset
+	option name 'rule-ads'
+	option type 'domain'
+	option source 'url'
+	option url 'https://example.invalid/list.txt'
+
+config rule
+	option name 'ads'
+	list dst_domain 'ads.example'
+	list dst_ruleset 'rule-ads'
+	option target 'block'
+`)
+	if err := migrateWith(f); err != nil {
+		t.Fatalf("migrate: %v", err)
+	}
+	// The operator's list is untouched...
+	if got := f.ruleset("rule-ads").opts["source"]; got != "url" {
+		t.Fatalf("the hand-written list was overwritten (source = %q)", got)
+	}
+	if got := f.ruleset("rule-ads").lists["entry"]; len(got) != 0 {
+		t.Fatalf("entries were injected into the hand-written list: %q", got)
+	}
+	// ...and the legacy entry landed in a new list of its own.
+	rs := f.ruleset("rule-ads-2")
+	if rs == nil {
+		t.Fatalf("legacy entry was dropped; rulesets = %q", f.rulesetNames())
+	}
+	if !eqStrings(rs.lists["entry"], []string{"full:ads.example"}) {
+		t.Fatalf("entries = %q", rs.lists["entry"])
+	}
+	// Both references stand: rule_set is ORed, so the operator's list keeps
+	// matching everything it matched before.
+	if !eqStrings(f.rule(0).lists["dst_ruleset"], []string{"rule-ads", "rule-ads-2"}) {
+		t.Fatalf("dst_ruleset = %q", f.rule(0).lists["dst_ruleset"])
+	}
+	if _, ok := f.rule(0).lists["dst_domain"]; ok {
+		t.Fatal("dst_domain survived a completed migration")
+	}
+}
+
+// A legacy list whose values are all blank produces no rule-set — so the option
+// must NOT be deleted. Deleting it left the rule with zero matchers, which is the
+// spelling of a catch-all: a rule that previously matched nothing would have
+// become the router's default route. Keeping it leaves the rule visibly
+// unmigrated, which the parser then holds disabled.
+func TestMigrate1to2KeepsALegacyListThatMigratesToNothing(t *testing.T) {
+	f := newFakeUCI(`package shater
+
+config globals 'globals'
+	option schema_version '1'
+
+config rule
+	option name 'blank'
+	list dst_domain '  '
+	option target 'direct'
+
+config rule
+	option name 'default'
+	option target 'block'
+`)
+	if err := migrateWith(f); err != nil {
+		t.Fatalf("migrate: %v", err)
+	}
+	if names := f.rulesetNames(); len(names) != 0 {
+		t.Fatalf("an empty rule-set was created: %q", names)
+	}
+	if got := f.rule(0).lists["dst_domain"]; !eqStrings(got, []string{"  "}) {
+		t.Fatalf("dst_domain = %q, want it left in place", got)
+	}
+
+	text, _ := f.Export("shater")
+	m, err := ParseUCIExport(text)
+	if err != nil {
+		t.Fatalf("parse: %v", err)
+	}
+	if IsCatchAll(m.Rules[0]) {
+		t.Fatal("a rule with an unmigrated dst_domain became a catch-all")
+	}
+	if m.Rules[0].Enabled {
+		t.Fatal("a rule with an unmigrated dst_domain stayed enabled")
+	}
+	// And it does not take the default route away from the rule that owns it.
+	if !IsCatchAll(m.Rules[1]) || !m.Rules[1].Enabled {
+		t.Fatal("the real catch-all was disturbed")
+	}
+}
+
+// An UNMIGRATED config (the migration never ran, or could not commit) must not
+// silently reroute the whole router. This is the failure the parser guard exists
+// for: `list dst_domain 'bank.ru'` + `option target 'direct'` used to parse as a
+// rule with no matchers at all, i.e. a catch-all, and generate points route Final
+// at the LAST catch-all — so one uncommitted `uci commit` sent every packet out
+// the plain WAN with the router's real address.
+func TestUnmigratedRuleIsNeverACatchAll(t *testing.T) {
+	const v1 = `package shater
+
+config globals 'globals'
+	option schema_version '1'
+
+config rule
+	option name 'bank'
+	option enabled '1'
+	option order '10'
+	list dst_domain 'bank.ru'
+	option target 'direct'
+
+config rule
+	option name 'ips'
+	option enabled '1'
+	option order '20'
+	list dst_ip '10.0.0.0/8'
+	option target 'group:corp'
+
+config rule
+	option name 'default'
+	option enabled '1'
+	option order '100'
+	option target 'group:auto'
+`
+	m, err := ParseUCIExport(v1)
+	if err != nil {
+		t.Fatalf("parse: %v", err)
+	}
+	if len(m.Rules) != 3 {
+		t.Fatalf("rules = %d, want 3", len(m.Rules))
+	}
+	for _, i := range []int{0, 1} {
+		r := m.Rules[i]
+		if len(r.LegacyDst) == 0 {
+			t.Fatalf("rule %q: the live legacy option went unnoticed", r.Name)
+		}
+		if IsCatchAll(r) {
+			t.Fatalf("rule %q became a catch-all — it would take over route Final", r.Name)
+		}
+		if r.Enabled {
+			t.Fatalf("rule %q stayed enabled with an unreadable destination", r.Name)
+		}
+	}
+	if m.Rules[0].LegacyDst[0] != "dst_domain=bank.ru" || m.Rules[1].LegacyDst[0] != "dst_ip=10.0.0.0/8" {
+		t.Fatalf("LegacyDst = %q / %q", m.Rules[0].LegacyDst, m.Rules[1].LegacyDst)
+	}
+	// The real default is untouched, and it is the only one.
+	if !IsCatchAll(m.Rules[2]) || !m.Rules[2].Enabled {
+		t.Fatal("the genuine catch-all was disturbed")
+	}
+
+	// The operator is told, through the ordinary config-warning channel.
+	warns := ValidateRules(m.Rules, nil)
+	if len(warns) != 2 {
+		t.Fatalf("warnings = %d (%v), want one per unmigrated rule", len(warns), warns)
+	}
+	for _, w := range warns {
+		if w.Section != "rule" || !strings.Contains(w.Message, "shaterd migrate") {
+			t.Fatalf("warning does not point at the fix: %+v", w)
+		}
+	}
+
+	// A profile may not hand such a rule its Enabled bit back either.
+	prof := &Profile{Name: "home", EnableRules: []string{"bank"}}
+	got, pwarns := ApplyProfileRuleOverrides(m.Rules, prof)
+	if got[0].Enabled {
+		t.Fatal("a profile re-enabled an unmigrated rule")
+	}
+	if len(pwarns) != 1 {
+		t.Fatalf("profile warnings = %v, want one refusal", pwarns)
+	}
+
+	// And after the migration the same config is fully live again.
+	f := newFakeUCI(v1)
+	if err := migrateWith(f); err != nil {
+		t.Fatalf("migrate: %v", err)
+	}
+	text, _ := f.Export("shater")
+	mm, err := ParseUCIExport(text)
+	if err != nil {
+		t.Fatalf("parse migrated: %v", err)
+	}
+	for i, r := range mm.Rules {
+		if len(r.LegacyDst) != 0 {
+			t.Fatalf("rule %d still flagged unmigrated: %q", i, r.LegacyDst)
+		}
+		if !r.Enabled {
+			t.Fatalf("rule %d stayed disabled after a successful migration", i)
+		}
+	}
+	if len(ValidateRules(mm.Rules, nil)) != 0 {
+		t.Fatalf("a migrated config still warns: %v", ValidateRules(mm.Rules, nil))
+	}
+}
+
+// failingUCI fails one named write, so the abort path can be observed.
+type failingUCI struct {
+	*fakeUCI
+	failAddList bool
+	failDelete  bool
+}
+
+var errFake = errors.New("uci: no space left on device")
+
+func (f *failingUCI) AddList(k, v string) error {
+	if f.failAddList {
+		return errFake
+	}
+	return f.fakeUCI.AddList(k, v)
+}
+
+func (f *failingUCI) Delete(k string) error {
+	if f.failDelete && strings.Contains(k, "dst_") {
+		return errFake
+	}
+	return f.fakeUCI.Delete(k)
+}
+
+// A failed write must abort the migration AND drop the staged half-migration.
+// Without the revert the partial rewrite sits in /tmp/.uci until the next
+// `uci commit shater` from any process (the panel saving one setting) flushes a
+// config that is half v1 and half v2 onto the disk.
+func TestMigrate1to2RevertsOnWriteFailure(t *testing.T) {
+	f := &failingUCI{fakeUCI: newFakeUCI(legacyConfig), failAddList: true}
+	err := migrateWith(f)
+	if err == nil {
+		t.Fatal("expected the failed write to abort the migration")
+	}
+	if f.reverts == 0 {
+		t.Fatal("the staged half-migration was left behind (no revert)")
+	}
+	if f.commits != 0 {
+		t.Fatalf("committed %d times despite the failure", f.commits)
+	}
+	if v, _ := f.Get("shater.globals.schema_version"); v != "1" {
+		t.Fatalf("schema_version = %q — a failed migration must not claim v2", v)
+	}
+}
+
+// A delete that fails must abort too. Swallowing it left the legacy option in the
+// config forever: migrateWith bumps schema_version to 2 on return, and the step
+// that would have removed it never runs again.
+func TestMigrate1to2FailsLoudlyWhenALegacyOptionCannotBeDeleted(t *testing.T) {
+	f := &failingUCI{fakeUCI: newFakeUCI(legacyConfig), failDelete: true}
+	if err := migrateWith(f); err == nil {
+		t.Fatal("a failed delete was swallowed")
+	}
+	if f.reverts == 0 {
+		t.Fatal("no revert after the failed delete")
+	}
+	if v, _ := f.Get("shater.globals.schema_version"); v == "2" {
+		t.Fatal("schema_version reached v2 with a legacy option still in the config")
+	}
+}
+
 // A rule carrying BOTH lists gets both rule-sets, and keeps every entry.
 func TestMigrate1to2SplitsMixedRuleIntoTwoRulesets(t *testing.T) {
 	f := newFakeUCI(`package shater

shater/model/model.go

@@ -654,6 +654,31 @@ type Rule struct {
 	Target     string // chain:|group:|node:|direct|block
 	Egress     string
 
+	// LegacyDst is the UNMIGRATED-RULE TRIPWIRE, and it is a safety device, not a
+	// data field. It is non-empty exactly when the config STILL carries a
+	// schema-v1 `dst_domain`/`dst_ip` on this rule — i.e. migrate1to2 never ran,
+	// or ran and could not commit (a full /overlay is the documented way that
+	// happens). Each element is the raw `<option>=<value>` text, purely so the
+	// warning can quote what it found.
+	//
+	// WHY IT EXISTS. Nothing re-runs the migration on the paths that matter: the
+	// daemon's `run`, the SIGHUP reconcile and the panel's config write all load
+	// UCI directly. The parser no longer reads the two removed options, so an
+	// unmigrated `list dst_domain 'bank.ru'` + `option target 'direct'` parsed as
+	// a rule with NO matchers at all — a catch-all, which generate turns into
+	// route `Final`, which the LAST such rule wins. One un-committed migration
+	// therefore sent the WHOLE router's traffic out the plain WAN, silently.
+	//
+	// WHAT IT DOES. ParseUCIExport forces Enabled=false on such a rule (generate
+	// and the reachability analysis both skip disabled rules), IsCatchAll reports
+	// false for it whatever its matchers say (so it can never become the default
+	// route even if something re-enables it), ApplyProfileRuleOverrides refuses to
+	// enable it, and ValidateRules reports it through the normal apply-warning
+	// channel. It is never rendered back to UCI: render.go emits neither legacy
+	// option, so a panel write drains them out — with `enabled '0'` recorded, so
+	// the rule stays inert until an operator looks at it.
+	LegacyDst []string
+
 	// Kill is the per-rule policy for when the target cannot resolve at generate
 	// time (dead group, chain that would not assemble, missing egress/node). The
 	// rule is ALWAYS still emitted — its traffic never falls through to the

shater/model/profile.go

@@ -114,6 +114,22 @@ func ApplyProfileRuleOverrides(rules []Rule, prof *Profile) ([]Rule, []Warning)
 				continue
 			}
 			for _, i := range targets {
+				// A profile may switch a rule OFF freely, but it may not switch an
+				// UNMIGRATED one on: its destination matcher is unreadable (see
+				// Rule.LegacyDst), so enabling it would put a rule into force with
+				// fewer conditions than the operator wrote — in the worst case none
+				// at all, i.e. the router's default route.
+				if enabled && len(out[i].LegacyDst) > 0 {
+					warns = append(warns, Warning{
+						Section: "profile",
+						Name:    prof.Name,
+						Message: fmt.Sprintf("cannot enable rule %q: it still carries the "+
+							"removed dst_domain/dst_ip options, so its destination cannot be "+
+							"read and it stays disabled until the config is migrated "+
+							"(run `shaterd migrate`)", n),
+					})
+					continue
+				}
 				out[i].Enabled = enabled
 			}
 		}

shater/model/reachability.go

@@ -50,9 +50,20 @@ import (
 // carried `dst_domain`/`dst_ip` is not, because the migration gives it a
 // DstRuleset in their place.
 //
+// AN UNMIGRATED RULE IS NEVER A CATCH-ALL. A rule that still carries a
+// schema-v1 `dst_domain`/`dst_ip` (Rule.LegacyDst) has a destination the parser
+// cannot read, so its lack of matchers here means "unreadable", not "everything".
+// Calling it a catch-all is what turned an uncommitted migration into route
+// `Final` for the whole router. ParseUCIExport already holds such a rule
+// disabled; this is the second lock, and it is the one that holds if anything
+// ever hands the rule back its Enabled bit.
+//
 // generate.isCatchAll and the panel's isCatchAll() are the same predicate; this
 // is the one the Go side shares.
 func IsCatchAll(r Rule) bool {
+	if len(r.LegacyDst) > 0 {
+		return false
+	}
 	return len(r.Src) == 0 &&
 		len(r.DstRuleset) == 0 &&
 		strings.TrimSpace(r.DstPort) == "" &&

shater/model/render.go

@@ -9,7 +9,9 @@ package model
 // the same uciRunner seam the migrations use).
 
 import (
+	"errors"
 	"fmt"
+	"reflect"
 	"strconv"
 	"strings"
 )
@@ -33,6 +35,14 @@ import (
 // ParseUCIExport already carries those defaults as concrete values, so the
 // panel's read→edit→write flow round-trips exactly. See render_test.go.
 //
+// Rule.LegacyDst IS emitted (as the `dst_domain`/`dst_ip` it names), because a
+// write over an unmigrated config must not erase the operator's lists. This
+// function is PURE and therefore trusts the Model it is given — so a Model whose
+// LegacyDst did not come from the config on disk would have it written to disk.
+// writeUCIWith is what guarantees the field's provenance (withDiskLegacyDst), and
+// it is the only non-test caller; any future caller that persists the result owes
+// the same guarantee.
+//
 // Subscription-cache nodes (FromSub != "") are NOT emitted at all: they live in
 // per-subscription JSON cache files (see subcache.go) and are folded back in by
 // ReadUCI's MergeSubCaches. Only manual nodes become `config node` sections, and
@@ -211,10 +221,11 @@ func RenderUCIExport(m *Model) string {
 		w.boolOpt("enabled", r.Enabled)
 		w.intOpt("order", r.Order)
 		w.listOpt("src", r.Src)
-		// dst_domain / dst_ip are NOT emitted (removed in schema v2). Their absence
-		// here is also how a legacy option drains out of a config that was migrated:
-		// migrate1to2 deletes them explicitly, and any that survived a hand-edit
-		// disappear the next time the panel writes the model back.
+		// dst_domain / dst_ip are never SYNTHESISED (they are not part of schema v2)
+		// — but they ARE written back when the rule still carries them, so a write
+		// that is allowed to proceed over an unmigrated config cannot erase them.
+		// See legacyDstOpts and guardUnmigratedConfig.
+		w.legacyDstOpts(r.LegacyDst)
 		w.listOpt("dst_ruleset", r.DstRuleset)
 		w.strOpt("dst_port", r.DstPort)
 		w.strOpt("proto", r.Proto)
@@ -378,6 +389,40 @@ func (w *uciWriter) listOpt(k string, vs []string) {
 	}
 }
 
+// legacyDstOpts re-emits the schema-v1 `dst_domain`/`dst_ip` a rule STILL carries
+// on disk (Rule.LegacyDst, `<option>=<value>` text produced by legacyDstEntries).
+//
+// This is preservation, not support. A write over an unmigrated config is only
+// ever allowed when it does not touch the rules at all (guardUnmigratedConfig) —
+// a subscription refresh writing quota counters, the profile watcher switching
+// the active profile. Those writers re-render the WHOLE package, so without this
+// the operator's destination lists would be erased by a cron job, which is the
+// same data loss the guard exists to prevent, just triggered from a different
+// place. Writing them back keeps the config exactly as unmigrated as it was, so
+// `shaterd migrate` can still do its job afterwards.
+//
+// Only the two known keys with a non-empty value are emitted: LegacyDst may be
+// filled by a client (the panel PUTs back the model it GETs), and this must not
+// become a way to inject arbitrary `list <anything>` lines into the config.
+func (w *uciWriter) legacyDstOpts(entries []string) {
+	for _, e := range entries {
+		i := strings.IndexByte(e, '=')
+		if i <= 0 {
+			continue
+		}
+		k, v := e[:i], e[i+1:]
+		if v == "" {
+			continue
+		}
+		for _, known := range legacyDstOptions {
+			if k == known {
+				fmt.Fprintf(&w.b, "\tlist %s %s\n", k, renderQuote(v))
+				break
+			}
+		}
+	}
+}
+
 // renderQuote single-quotes a value and escapes embedded single quotes the uci
 // way (`'\”`), mirroring uci.go's unquote.
 //
@@ -425,8 +470,170 @@ func WriteUCI(m *Model) error {
 	return writeUCIWith(uci, m)
 }
 
+// ErrUnmigratedConfig is what every config write fails with when the config ON
+// DISK still carries the schema-v1 rule destinations and the write would change
+// the rules. Callers match it with errors.Is to tell this refusal (a config the
+// operator can fix with one command) apart from a real I/O failure — the panel
+// answers 409 with the message rather than a bare 500.
+var ErrUnmigratedConfig = errors.New("config not migrated to schema v2")
+
+// CheckConfigWritable reports whether persisting m would be refused, WITHOUT
+// writing anything. It is the same check WriteUCI runs, exported so a caller that
+// does several writes in one request can find out before the first of them lands:
+// the panel's PUT /api/config writes the subscription node caches first, and
+// discovering the refusal only at the UCI step would leave those caches rewritten
+// for a request that was rejected.
+func CheckConfigWritable(m *Model) error {
+	disk, ok := diskRules(uci)
+	return guardUnmigratedConfig(disk, ok, m)
+}
+
+// diskRules returns the rules of the config CURRENTLY on disk, and whether they
+// could be read at all. ok=false covers a fresh install (no package) and an
+// export that will not parse — the two cases where there is nothing on disk to
+// protect and nothing to copy from.
+//
+// It is read ONCE per write and handed to both guardUnmigratedConfig and
+// withDiskLegacyDst: they answer two halves of the same question ("may this write
+// proceed?" / "what may it say about the legacy options?") and must not be able
+// to see different configs.
+func diskRules(u uciRunner) ([]Rule, bool) {
+	text, ok := u.Export("shater")
+	if !ok || strings.TrimSpace(text) == "" {
+		return nil, false
+	}
+	disk, err := ParseUCIExport(text)
+	if err != nil {
+		return nil, false
+	}
+	return disk.Rules, true
+}
+
+// withDiskLegacyDst returns m with every rule's LegacyDst forced to what the DISK
+// says, so the renderer can never be told about a legacy option that is not
+// really there. m is not modified: the rules are copied when (and only when)
+// something actually differs.
+//
+// WHY. LegacyDst is a safety device, and PUT /api/config decodes the whole Model
+// out of the request body — including this field. On a healthy, migrated config
+// the guard above returns early (there is nothing to protect), and the renderer
+// would then have written a fabricated `list dst_domain 'example.com'` straight
+// into /etc/config/shater. No traffic leak — everything downstream fails closed —
+// but an authenticated client could switch off any rule it named and lock the box
+// out of saving its config until someone ran `shaterd migrate`, and the warning
+// explaining it would have blamed a migration that never had anything to do with
+// it. Taking the value from disk removes the input entirely.
+//
+// In the branch where the disk DOES carry legacy options, the guard has already
+// established reflect.DeepEqual(disk.Rules, m.Rules), so the substitution is a
+// no-op there by construction — this is purely the sanitiser for everything else.
+// When the disk is unreadable there is nothing to preserve, so every LegacyDst is
+// cleared: a fabricated one must never be the reason an option appears on disk.
+func withDiskLegacyDst(disk []Rule, ok bool, m *Model) *Model {
+	if m == nil {
+		return m
+	}
+	want := func(i int) []string {
+		if !ok || i >= len(disk) {
+			return nil
+		}
+		return disk[i].LegacyDst
+	}
+	differs := false
+	for i := range m.Rules {
+		if !reflect.DeepEqual(m.Rules[i].LegacyDst, want(i)) {
+			differs = true
+			break
+		}
+	}
+	if !differs {
+		return m
+	}
+	out := *m
+	out.Rules = append([]Rule(nil), m.Rules...)
+	for i := range out.Rules {
+		out.Rules[i].LegacyDst = want(i)
+	}
+	return &out
+}
+
+// guardUnmigratedConfig refuses a write that would silently destroy schema-v1
+// rule destinations still present on disk.
+//
+// WHY IT READS THE DISK AND NOT THE MODEL. The Model handed to WriteUCI is not
+// trustworthy: PUT /api/config decodes one straight out of the request body, and
+// the body belongs to the client (an older panel build, curl, a script). A check
+// phrased as "refuse when m has LegacyDst" is defeated by simply omitting the
+// field — and that omission is precisely the dangerous request, because
+// RenderUCIExport would then write the rule with `enabled '1'` and NO destination
+// at all, which the next read parses as a catch-all and generate turns into route
+// `Final` for the entire router. Only the config already on disk can say whether
+// there is anything to protect, so that is what is consulted.
+//
+// WHAT IT ALLOWS. A write whose rules are IDENTICAL to the ones on disk goes
+// through, and legacyDstOpts writes the legacy options back with it. That keeps
+// the non-panel writers working on an unmigrated box — a subscription refresh
+// persisting quota counters and manual nodes (apply.refreshSubscription,
+// `shaterd sub update`) and the profile watcher switching globals.active_profile
+// — none of which has any business editing rules. They all build their Model with
+// ReadUCI, so their rules ARE the disk's, byte for byte, including the LegacyDst
+// the parser attached. Nothing about a migrated config changes: with no legacy
+// options on disk the function returns nil before comparing anything.
+//
+// An unreadable/unparseable export is treated as "nothing to protect" (ok=false
+// from diskRules). It cannot be distinguished from a fresh install here, and
+// refusing every write on a malformed file would leave the box unconfigurable
+// through its only UI.
+func guardUnmigratedConfig(disk []Rule, ok bool, m *Model) error {
+	if !ok {
+		return nil // fresh install / unreadable: no config to lose
+	}
+	var stuck []string
+	for i, r := range disk {
+		if len(r.LegacyDst) == 0 {
+			continue
+		}
+		if r.Name != "" {
+			stuck = append(stuck, strconv.Quote(r.Name))
+		} else {
+			stuck = append(stuck, "#"+strconv.Itoa(i))
+		}
+	}
+	if len(stuck) == 0 {
+		return nil // migrated (the normal case): nothing to guard
+	}
+	if m != nil && reflect.DeepEqual(disk, m.Rules) {
+		return nil // the rules are untouched; legacyDstOpts carries them across
+	}
+	return fmt.Errorf("%w: rule %s still carr%s the removed dst_domain/dst_ip "+
+		"options, and this config has no place to write them — saving would erase "+
+		"the destination lists and leave the rule matching nothing (or, worse, "+
+		"everything). The write was REFUSED and nothing on disk changed. Run "+
+		"`shaterd migrate` on the router to fold each list into a rule-set, then "+
+		"reload and save again.",
+		ErrUnmigratedConfig, strings.Join(stuck, ", "), plural(len(stuck), "ies", "y"))
+}
+
+// plural picks the suffix for a count (carries/carry).
+func plural(n int, one, many string) string {
+	if n == 1 {
+		return one
+	}
+	return many
+}
+
 func writeUCIWith(u uciRunner, m *Model) error {
-	text := RenderUCIExport(m)
+	// ONE read of the disk feeds both halves of the protection below.
+	disk, ok := diskRules(u)
+	// Refuse BEFORE the delete+import: writeUCIWith replaces the whole package, so
+	// by the time an import has run the legacy options are already gone.
+	if err := guardUnmigratedConfig(disk, ok, m); err != nil {
+		return err
+	}
+	// Render from the DISK's idea of which rules carry legacy options, never the
+	// caller's — RenderUCIExport is a pure function and will faithfully emit a
+	// `list dst_domain` for anything it is told about.
+	text := RenderUCIExport(withDiskLegacyDst(disk, ok, m))
 	var errs []string
 	// Clear the staged package first so `uci import` replaces rather than merges
 	// (import appends sections; without the delete a second write would duplicate

shater/model/render_test.go

@@ -1,6 +1,7 @@
 package model
 
 import (
+	"errors"
 	"reflect"
 	"strings"
 	"testing"
@@ -417,3 +418,210 @@ func TestWriteUCIReplaces(t *testing.T) {
 		t.Fatalf("second write duplicated sections: nodes=%d rules=%d", len(got2.Nodes), len(got2.Rules))
 	}
 }
+
+// --- write-path guard: an unmigrated config may not be overwritten ------------
+
+// unmigratedOnDisk is a v1 config as it sits on the router when `shaterd migrate`
+// never got to commit: two rules whose destination is still an inline list, plus
+// a subscription whose quota counters a refresh wants to update.
+const unmigratedOnDisk = `package shater
+
+config globals 'globals'
+	option enabled '1'
+	option schema_version '1'
+
+config subscription
+	option name 'qomar'
+	option url 'https://example.invalid/sub'
+
+config rule
+	option name 'bank'
+	option enabled '1'
+	list dst_domain 'bank.ru'
+	option target 'direct'
+
+config rule
+	option name 'default'
+	option enabled '1'
+	option target 'group:auto'
+`
+
+// A write that would change the rules of an unmigrated config is REFUSED, and the
+// on-disk config is byte-identical afterwards. Without this the renderer (which
+// emits no dst_domain/dst_ip) simply dropped the operator's lists on the first
+// save from the panel.
+func TestWriteUCIRefusesToOverwriteAnUnmigratedConfig(t *testing.T) {
+	f := newFakeUCI(unmigratedOnDisk)
+	before, _ := f.Export("shater")
+
+	m, err := ParseUCIExport(before)
+	if err != nil {
+		t.Fatalf("parse: %v", err)
+	}
+	m.Rules[0].Order = 42 // any rule edit at all
+
+	err = writeUCIWith(f, m)
+	if err == nil {
+		t.Fatal("the write was allowed to erase the legacy destination lists")
+	}
+	if !errors.Is(err, ErrUnmigratedConfig) {
+		t.Fatalf("error is not ErrUnmigratedConfig: %v", err)
+	}
+	for _, want := range []string{`"bank"`, "dst_domain", "shaterd migrate", "REFUSED"} {
+		if !strings.Contains(err.Error(), want) {
+			t.Fatalf("message does not mention %q: %s", want, err)
+		}
+	}
+	if after, _ := f.Export("shater"); after != before {
+		t.Fatalf("the config on disk changed despite the refusal:\n--- before\n%s\n--- after\n%s", before, after)
+	}
+	if f.commits != 0 || len(f.deleted) != 0 {
+		t.Fatalf("the refused write still touched uci (commits=%d deleted=%v)", f.commits, f.deleted)
+	}
+}
+
+// The guard reads the DISK, not the submitted model — so a body that simply omits
+// LegacyDst and sets Enabled cannot get a destination-less rule written with
+// `enabled '1'`. That rule would parse back as a catch-all and become route Final
+// for the whole router, which is the leak the parser guard closes on the read
+// side and this closes on the write side.
+func TestWriteUCIRefusesACraftedModelWithoutLegacyDst(t *testing.T) {
+	f := newFakeUCI(unmigratedOnDisk)
+	before, _ := f.Export("shater")
+
+	// Exactly what a hand-rolled PUT (or an older panel build) sends: the rule is
+	// there, enabled, and the field that marks it unmigrated is gone.
+	crafted := &Model{
+		Globals: DefaultGlobals(),
+		Rules: []Rule{
+			{Name: "bank", Enabled: true, Target: "direct"},
+			{Name: "default", Enabled: true, Target: "group:auto"},
+		},
+	}
+	if err := writeUCIWith(f, crafted); !errors.Is(err, ErrUnmigratedConfig) {
+		t.Fatalf("crafted body was accepted (err = %v)", err)
+	}
+	after, _ := f.Export("shater")
+	if after != before {
+		t.Fatalf("disk changed:\n--- before\n%s\n--- after\n%s", before, after)
+	}
+	if strings.Contains(after, "config rule\n\toption name 'bank'\n\toption enabled '1'\n\toption target") {
+		t.Fatal("a destination-less enabled rule reached the disk")
+	}
+	// And re-reading the disk still yields the protected shape.
+	m, err := ParseUCIExport(after)
+	if err != nil {
+		t.Fatalf("parse: %v", err)
+	}
+	if IsCatchAll(m.Rules[0]) || m.Rules[0].Enabled {
+		t.Fatal("the unmigrated rule lost its protection")
+	}
+}
+
+// A writer that does NOT touch the rules keeps working on an unmigrated box, and
+// the legacy options survive the write. This is the subscription-refresh path
+// (apply.UpdateSubscription / `shaterd sub update`) and the profile watcher: they
+// build their model with ReadUCI, change a counter or globals.active_profile, and
+// re-render the WHOLE package — so a blanket refusal would break a cron job, and
+// a blanket allow would let that cron job erase the operator's lists.
+func TestWriteUCIPreservesLegacyDstOnANonRuleWrite(t *testing.T) {
+	f := newFakeUCI(unmigratedOnDisk)
+	text, _ := f.Export("shater")
+	m, err := ParseUCIExport(text)
+	if err != nil {
+		t.Fatalf("parse: %v", err)
+	}
+	// Exactly what subscribe.StoreUserInfo does.
+	m.Subscriptions[0].UserDownload = 1 << 40
+	m.Subscriptions[0].UserInfoAt = 1700000000
+	m.Globals.ActiveProfile = "home"
+
+	if err := writeUCIWith(f, m); err != nil {
+		t.Fatalf("a non-rule write was refused: %v", err)
+	}
+
+	after, _ := f.Export("shater")
+	got, err := ParseUCIExport(after)
+	if err != nil {
+		t.Fatalf("re-parse: %v", err)
+	}
+	if got.Subscriptions[0].UserDownload != 1<<40 || got.Globals.ActiveProfile != "home" {
+		t.Fatalf("the write did not land: %+v / %q", got.Subscriptions[0], got.Globals.ActiveProfile)
+	}
+	if !eqStrings(got.Rules[0].LegacyDst, []string{"dst_domain=bank.ru"}) {
+		t.Fatalf("the legacy destination list was erased: %q", got.Rules[0].LegacyDst)
+	}
+	if got.Rules[0].Enabled || IsCatchAll(got.Rules[0]) {
+		t.Fatal("the rule lost its unmigrated protection across the write")
+	}
+	// The config is still exactly as unmigrated as it was, so `shaterd migrate`
+	// can still fold the list into a rule-set afterwards.
+	if err := migrate1to2(f); err != nil {
+		t.Fatalf("migrate after the write: %v", err)
+	}
+	if f.ruleset("rule-bank") == nil {
+		t.Fatalf("migration found nothing to fold; rulesets = %q", f.rulesetNames())
+	}
+}
+
+// A fabricated LegacyDst in the submitted model must never reach the disk. On a
+// MIGRATED config the guard has nothing to protect and returns early, so without
+// withDiskLegacyDst the renderer happily wrote the client's `list dst_domain`
+// into /etc/config/shater — and from there every downstream lock fired on a lie:
+// the rule the client named went (and stayed) disabled, further rule writes were
+// refused, and the warning blamed a migration that had never been involved.
+func TestWriteUCIIgnoresAFabricatedLegacyDst(t *testing.T) {
+	const migratedOnDisk = `package shater
+
+config globals 'globals'
+	option enabled '1'
+	option schema_version '2'
+
+config rule
+	option name 'bank'
+	option enabled '1'
+	list dst_ruleset 'rule-bank'
+	option target 'direct'
+`
+	f := newFakeUCI(migratedOnDisk)
+	text, _ := f.Export("shater")
+	m, err := ParseUCIExport(text)
+	if err != nil {
+		t.Fatalf("parse: %v", err)
+	}
+	if len(m.Rules[0].LegacyDst) != 0 {
+		t.Fatal("fixture is not migrated")
+	}
+	// Exactly what a crafted PUT body carries.
+	m.Rules[0].LegacyDst = []string{"dst_domain=example.com", "dst_ip=203.0.113.0/24"}
+
+	if err := writeUCIWith(f, m); err != nil {
+		t.Fatalf("writeUCIWith: %v", err)
+	}
+	after, _ := f.Export("shater")
+	for _, forbidden := range []string{"dst_domain", "dst_ip"} {
+		if strings.Contains(after, forbidden) {
+			t.Fatalf("a fabricated %s reached the disk:\n%s", forbidden, after)
+		}
+	}
+	got, err := ParseUCIExport(after)
+	if err != nil {
+		t.Fatalf("re-parse: %v", err)
+	}
+	if len(got.Rules[0].LegacyDst) != 0 {
+		t.Fatalf("rule came back unmigrated: %q", got.Rules[0].LegacyDst)
+	}
+	// Enabled is whatever was submitted — the fabrication must not have switched
+	// the rule off either.
+	if !got.Rules[0].Enabled {
+		t.Fatal("the fabricated field disabled a healthy rule")
+	}
+	// And the caller's model was not mutated behind its back.
+	if len(m.Rules[0].LegacyDst) != 2 {
+		t.Fatalf("writeUCIWith mutated the caller's model: %q", m.Rules[0].LegacyDst)
+	}
+	// The config is still writable: nothing latched.
+	if err := writeUCIWith(f, got); err != nil {
+		t.Fatalf("the config became unwritable: %v", err)
+	}
+}

shater/model/roundtrip_systematic_test.go

@@ -74,6 +74,14 @@ func fullModel() *Model {
 	m.Nodes[0].FromSub = ""
 	m.Nodes[0].Fingerprint = ""
 	m.Nodes[0].Stale = false
+	// Rule.LegacyDst is exempt because it is not a field with a value of its own:
+	// each element is `<option>=<value>` naming one of exactly two removed options,
+	// and uciWriter.legacyDstOpts deliberately drops anything else so a client
+	// cannot inject arbitrary `list` lines through it. fillNonZero's generic
+	// "s-legacydst-1" is precisely such an anything-else, so it cannot round-trip
+	// by construction. The real round-trip (parse a legacy option -> render it back
+	// unchanged) is pinned by TestWriteUCIPreservesLegacyDstOnANonRuleWrite.
+	m.Rules[0].LegacyDst = nil
 	return m
 }
 

shater/model/uci.go

@@ -174,18 +174,38 @@ func ParseUCIExport(text string) (*Model, error) {
 				Entries:        s.list("entry"),
 			})
 		case "rule":
+			// dst_domain / dst_ip were REMOVED in schema v2: a rule's destination is
+			// a rule-set reference and nothing else, and migrate1to2 (migrate.go)
+			// folds any legacy inline list into a `config ruleset` and points
+			// dst_ruleset at it.
+			//
+			// The migration is NOT re-run on every load — it runs from the service
+			// init and from uci-defaults at package install (`shaterd migrate`), and
+			// from nowhere else: the daemon's `run`, the SIGHUP reconcile and the
+			// panel's config write all come straight here. So a config that still
+			// carries the options is a real, reachable state — an interrupted or
+			// uncommittable migration (a full /overlay is the documented way that
+			// happens), or a hand edit after a downgrade.
+			//
+			// Ignoring them there was NOT safe. A rule whose only matcher was
+			// `dst_domain` parsed as a rule with NO matchers, which IS the spelling
+			// of a catch-all: generate points route `Final` at it and the last such
+			// rule wins, so `dst_domain bank.ru` + `target direct` quietly became
+			// "send EVERYTHING out the plain WAN". legacyDstEntries detects the live
+			// options; a rule that has them is held DISABLED here and is reported by
+			// ValidateRules, and Rule.LegacyDst keeps IsCatchAll/profile overrides
+			// from resurrecting it. See the field's doc comment in model.go.
+			legacyDst := legacyDstEntries(s)
 			m.Rules = append(m.Rules, Rule{
-				Name:    firstNonEmpty(s.opt("name"), s.Name),
-				Enabled: s.optBool("enabled", true),
-				Order:   parseInt(s.opt("order"), 0),
-				Src:     s.list("src"),
-				// No dst_domain / dst_ip: removed in schema v2. A rule's destination
-				// is a rule-set reference and nothing else; migrate1to2 (migrate.go)
-				// converts any legacy inline list into a `config ruleset` and points
-				// dst_ruleset at it, so by the time this parser runs there is nothing
-				// left to read. A config that somehow still carries them (hand-edited
-				// after a downgrade) simply ignores them — the migration is re-run on
-				// every load, so it will have been rewritten first.
+				Name: firstNonEmpty(s.opt("name"), s.Name),
+				// An unmigrated rule is never in force. Its destination matcher is
+				// unreadable, so every alternative — routing it without the matcher,
+				// or treating the absence as "matches everything" — states a policy
+				// the operator did not write.
+				Enabled:      s.optBool("enabled", true) && len(legacyDst) == 0,
+				Order:        parseInt(s.opt("order"), 0),
+				Src:          s.list("src"),
+				LegacyDst:    legacyDst,
 				DstRuleset:   s.list("dst_ruleset"),
 				DstPort:      s.opt("dst_port"),
 				Proto:        s.opt("proto"),
@@ -340,6 +360,40 @@ func applyGlobals(g *Globals, s uciSection) {
 	g.StatsDiskLimitMB = parseInt(s.opt("stats_disk_limit_mb"), g.StatsDiskLimitMB)
 }
 
+// legacyDstOptions are the `config rule` options schema v2 removed. Their mere
+// PRESENCE on a rule proves the config was not migrated (migrate1to2 deletes them
+// as its last step per rule, and only once the replacement rule-set reference is
+// in place).
+var legacyDstOptions = []string{"dst_domain", "dst_ip"}
+
+// legacyDstEntries returns the live schema-v1 destination entries of a rule
+// section as `<option>=<value>` text, or nil when there are none.
+//
+// PRESENCE, not content, is the test. A `list dst_domain '  '` is kept as an
+// entry even though it names no domain: the value is junk, but the OPTION being
+// there still means the migration did not process this rule, and dropping the
+// blank would hand the rule back its catch-all shape — the exact leak this
+// detection exists to stop. (A truly empty `list dst_domain ”` never reaches
+// here: parseSections drops empty list values, so the key is simply absent, and
+// such a rule had no destination under v1 either.)
+func legacyDstEntries(s uciSection) []string {
+	var out []string
+	for _, k := range legacyDstOptions {
+		vals, ok := s.Lists[k]
+		if !ok {
+			continue
+		}
+		if len(vals) == 0 {
+			out = append(out, k+"=")
+			continue
+		}
+		for _, v := range vals {
+			out = append(out, k+"="+v)
+		}
+	}
+	return out
+}
+
 // --- section accessors ---
 
 func (s uciSection) opt(k string) string { return s.Options[k] }

shater/model/validate.go

@@ -123,6 +123,28 @@ func ValidateRules(rules []Rule, classify NetClassifier) []Warning {
 	}
 	var out []Warning
 	for _, r := range rules {
+		// Checked BEFORE the Enabled gate on purpose: the parser holds an unmigrated
+		// rule disabled, so gating on Enabled would suppress the one warning that
+		// explains why the rule stopped working.
+		if len(r.LegacyDst) > 0 {
+			out = append(out, Warning{
+				Section: "rule",
+				Name:    r.Name,
+				Message: fmt.Sprintf(
+					"this rule still carries the removed schema-v1 destination options "+
+						"(%s), so the config was never brought to schema v%d — either "+
+						"`shaterd migrate` never ran, or it ran and could not commit (a "+
+						"full /overlay is the usual cause), or the section was hand-edited. "+
+						"Its destination cannot be read, so the rule is HELD "+
+						"DISABLED: routing it without its destination would have applied "+
+						"target %q to far more traffic than you wrote it for, and a rule "+
+						"whose only matcher was one of these options would have become the "+
+						"router's DEFAULT ROUTE. Run `shaterd migrate` to convert them into "+
+						"a rule-set, then re-enable the rule.",
+					strings.Join(r.LegacyDst, ", "), CurrentSchemaVersion,
+					EffectiveRuleTarget(r)),
+			})
+		}
 		if !r.Enabled {
 			continue // a disabled rule installs nothing
 		}

shater/panel/api.go

@@ -203,6 +203,22 @@ func (s *Server) handleConfigPut(w http.ResponseWriter, r *http.Request) {
 		writeError(w, http.StatusBadRequest, err.Error())
 		return
 	}
+	// Refuse to overwrite a config that was never migrated to schema v2, BEFORE
+	// touching anything. The check reads the config on disk, not this body: a
+	// request that simply omits the LegacyDst field is exactly the dangerous one
+	// (it would have the renderer write a rule with `enabled '1'` and no
+	// destination, which the next read treats as the router's default route), so
+	// trusting the body would defeat the guard. Running it here, ahead of
+	// syncSubCaches, keeps a rejected request from rewriting the node caches.
+	// 409, not 500: nothing is broken, the operator has one command to run.
+	if err := checkWritable(&m); err != nil {
+		if errors.Is(err, model.ErrUnmigratedConfig) {
+			writeError(w, http.StatusConflict, err.Error())
+			return
+		}
+		writeError(w, http.StatusInternalServerError, "check config: "+err.Error())
+		return
+	}
 	// Caches first: they are plain files and the cheaper write; a UCI failure
 	// after them leaves provider-owned node caches fresh and the manual config
 	// untouched, which the next successful PUT converges anyway.
@@ -211,6 +227,13 @@ func (s *Server) handleConfigPut(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 	if err := writeConfig(&m); err != nil {
+		// The same refusal can still surface here (WriteUCI re-checks under its own
+		// read, so a migration that finished between the two calls cannot slip a
+		// destructive write through). Keep the status honest.
+		if errors.Is(err, model.ErrUnmigratedConfig) {
+			writeError(w, http.StatusConflict, err.Error())
+			return
+		}
 		writeError(w, http.StatusInternalServerError, "write config: "+err.Error())
 		return
 	}
@@ -231,6 +254,9 @@ const maxConfigBytes = 4 << 20 // 4 MiB
 var (
 	writeConfig   = model.WriteUCI
 	syncSubCaches = model.SyncSubCaches
+	// checkWritable is the pre-flight refusal (see handleConfigPut); it reads the
+	// on-disk config and never writes.
+	checkWritable = model.CheckConfigWritable
 )
 
 // validateModel does light structural validation — enough to reject obviously

shater/panel/config_put_test.go

@@ -3,6 +3,7 @@ package panel
 import (
 	"encoding/json"
 	"errors"
+	"fmt"
 	"io"
 	"net/http"
 	"net/http/httptest"
@@ -227,3 +228,81 @@ func readAll(resp *http.Response) (string, error) {
 	b, err := io.ReadAll(resp.Body)
 	return string(b), err
 }
+
+// A PUT over a config that was never migrated to schema v2 is REFUSED with 409
+// and a readable body, and nothing is written — not the UCI config and not the
+// subscription node caches (the guard runs ahead of both).
+//
+// The crafted body is the dangerous shape the coordinator flagged: the rule is
+// present and Enabled, and LegacyDst — the field that marks it unmigrated — is
+// simply absent. Trusting the body would have the renderer write `enabled '1'`
+// with no destination at all, which the next read parses as a catch-all and
+// generate turns into route Final for the entire router. The guard consults the
+// config on disk instead, which is why omitting the field changes nothing.
+func TestConfigPutRefusesUnmigratedConfig(t *testing.T) {
+	s := newTestServer(t)
+	srv := httptest.NewServer(s.Handler())
+	defer srv.Close()
+	cookie := login(t, srv, s)
+
+	wrote, synced := false, false
+	origWrite, origSync, origCheck := writeConfig, syncSubCaches, checkWritable
+	writeConfig = func(*model.Model) error { wrote = true; return nil }
+	syncSubCaches = func(*model.Model) error { synced = true; return nil }
+	checkWritable = func(*model.Model) error {
+		return fmt.Errorf("%w: rule \"bank\" still carries the removed dst_domain/dst_ip "+
+			"options ... The write was REFUSED and nothing on disk changed. Run "+
+			"`shaterd migrate` on the router", model.ErrUnmigratedConfig)
+	}
+	defer func() { writeConfig, syncSubCaches, checkWritable = origWrite, origSync, origCheck }()
+
+	body := `{
+	  "Globals": {"Enabled": true},
+	  "Rules": [{"Name": "bank", "Enabled": true, "Target": "direct"}]
+	}`
+	resp := doPut(t, srv, cookie, body)
+	defer resp.Body.Close()
+	if resp.StatusCode != http.StatusConflict {
+		b, _ := readAll(resp)
+		t.Fatalf("PUT over an unmigrated config: got %d, want 409 (body %s)", resp.StatusCode, b)
+	}
+	if wrote {
+		t.Fatal("the config was written despite the refusal")
+	}
+	if synced {
+		t.Fatal("the subscription caches were rewritten for a rejected request")
+	}
+	var out map[string]string
+	if err := json.NewDecoder(resp.Body).Decode(&out); err != nil {
+		t.Fatalf("decode error body: %v", err)
+	}
+	for _, want := range []string{"shaterd migrate", "REFUSED", "bank"} {
+		if !strings.Contains(out["error"], want) {
+			t.Fatalf("error body does not mention %q: %q", want, out["error"])
+		}
+	}
+}
+
+// The same refusal raised by WriteUCI itself (a migration that finished between
+// the pre-flight check and the write, or any caller that skipped the pre-flight)
+// must still reach the client as 409 with its text, not a bare 500.
+func TestConfigPutMapsWriteRefusalTo409(t *testing.T) {
+	s := newTestServer(t)
+	srv := httptest.NewServer(s.Handler())
+	defer srv.Close()
+	cookie := login(t, srv, s)
+
+	origWrite, origSync, origCheck := writeConfig, syncSubCaches, checkWritable
+	writeConfig = func(*model.Model) error {
+		return fmt.Errorf("%w: rule \"bank\" ...", model.ErrUnmigratedConfig)
+	}
+	syncSubCaches = func(*model.Model) error { return nil }
+	checkWritable = func(*model.Model) error { return nil }
+	defer func() { writeConfig, syncSubCaches, checkWritable = origWrite, origSync, origCheck }()
+
+	resp := doPut(t, srv, cookie, `{"Globals": {"Enabled": true}}`)
+	defer resp.Body.Close()
+	if resp.StatusCode != http.StatusConflict {
+		t.Fatalf("got %d, want 409", resp.StatusCode)
+	}
+}