docs(plan): script-editor ctx tag-completion correctness + truthfulness design
This commit is contained in:
@@ -0,0 +1,105 @@
|
||||
# Script-editor `ctx` tag-completion correctness + truthfulness — Design
|
||||
|
||||
> Brainstormed 2026-06-18. Backlog item: stillpending.md §4 "Monaco `ctx.SetVirtualTag(...)`
|
||||
> write-target completions/hover are best-effort" (`AdminUI/ScriptAnalysis/IScriptTagCatalog.cs`,
|
||||
> `ScriptAnalysisService.cs`). Off master `651018f9`.
|
||||
|
||||
## Problem (verified in code)
|
||||
|
||||
The Monaco script editor's tag-path completion + hover (ScriptEdit page and the inline virtual-tag
|
||||
modal) route through one private gate, `ScriptAnalysisService.TryGetTagPathLiteral(token, …)`. Two
|
||||
defects:
|
||||
|
||||
1. **Receiver-blind gate.** `TryGetTagPathLiteral` accepts ANY `<expr>.GetTag("…")` /
|
||||
`<expr>.SetVirtualTag("…")` — it matches the member-access *method name* only
|
||||
(`method is not ("GetTag" or "SetVirtualTag")`) and never inspects the receiver. So a stray
|
||||
`someOtherObject.GetTag("…")` wrongly triggers tag-path completion/hover. This makes the editor
|
||||
**more permissive than the runtime**, whose dependency harvest is syntactically `ctx`-anchored
|
||||
(`Commons/Types/EquipmentScriptPaths.GetTagRefRegex` = `ctx\s*\.\s*(?:GetTag|SetVirtualTag)\s*\(`,
|
||||
consumed by `Phase7Composer` to build `EquipmentVirtualTagPlan.DependencyRefs`).
|
||||
|
||||
2. **`SetVirtualTag` advertises a no-op.** `IScriptTagCatalog`/`ScriptTagCatalog` emits a virtual
|
||||
tag's leaf `Name` as a `SetVirtualTag` write-target completion, documented as "best-effort … the
|
||||
live resolution of virtual-tag cascade/write targets is unconfirmed." It is now CONFIRMED:
|
||||
**`ctx.SetVirtualTag(...)` is a no-op in production.** The active evaluator binding
|
||||
`Host/Engines/RoslynVirtualTagEvaluator.cs` runs in **single-tag mode** and *drops* every
|
||||
cross-tag write (logged: "cross-tag write to {Path} dropped (single-tag adapter)"). The cross-tag
|
||||
cascade engine that DOES honor `SetVirtualTag` (`Core.VirtualTags.VirtualTagEngine`,
|
||||
`OnScriptSetVirtualTag` validating the target against registered vtags) is **dormant** — not wired
|
||||
into the host. So there is **no production write-target resolution** to be "authoritative" about.
|
||||
|
||||
## Scope decision
|
||||
|
||||
The original audit framed a fix sub-goal as "return *authoritative* `SetVirtualTag` write-target
|
||||
paths." Given finding (2), that goal is **moot and explicitly dropped** — the cascade feature is
|
||||
dormant; no authoritative production key exists. This phase ships the genuinely-fixable, fully
|
||||
live-verifiable core:
|
||||
|
||||
- **Component 1 — receiver-type guard** (correctness)
|
||||
- **Component 2 — truthful `SetVirtualTag` hover** (honesty)
|
||||
|
||||
Both AdminUI-only. **No runtime, Commons/wire/proto, or EF change. No bUnit.**
|
||||
|
||||
## Component 1 — receiver-type guard
|
||||
|
||||
In `TryGetTagPathLiteral`, after the member-access (`ma`) and method-name checks, require the
|
||||
receiver to be the identifier `ctx`:
|
||||
|
||||
```csharp
|
||||
if (ma.Expression is not IdentifierNameSyntax { Identifier.ValueText: "ctx" }) return false;
|
||||
```
|
||||
|
||||
This deliberately mirrors the runtime regex (syntactic `ctx` anchor) so the editor accepts EXACTLY
|
||||
what `Phase7Composer` harvests — not a semantic "is the receiver a `ScriptContext`" check, which
|
||||
could accept a differently-named local that the runtime regex would never harvest. The guard lives
|
||||
in the single shared gate, so it fixes both `Complete` and `Hover` at once. Effect: `foo.GetTag("…")`
|
||||
no longer offers tag completions/hover; only `ctx.GetTag(…)` / `ctx.SetVirtualTag(…)` do.
|
||||
|
||||
## Component 2 — truthful `SetVirtualTag`
|
||||
|
||||
`TryGetTagPathLiteral` surfaces WHICH method matched (a small out-param / enum, e.g.
|
||||
`ScriptTagMethod { GetTag, SetVirtualTag }`). The `Hover` path, when the matched method is
|
||||
`SetVirtualTag`, appends a clear note to the hover markdown:
|
||||
|
||||
> ⚠ Cross-tag `SetVirtualTag` writes are currently dropped in single-tag mode (the cascade engine is
|
||||
> not wired into the host); this write will not take effect at runtime.
|
||||
|
||||
So an author is no longer silently misled into relying on a no-op. The completion list still helps
|
||||
type a path (the truthfulness signal is the hover). Restricting `SetVirtualTag` *suggestions* to
|
||||
virtual-tag targets only is a possible refinement left to the implementation plan — NOT a hard
|
||||
requirement — because the dormant-engine write-target key is itself unverified (and the whole write
|
||||
is dropped), so over-engineering the suggestion set adds risk without runtime payoff.
|
||||
|
||||
## Error handling
|
||||
|
||||
Unchanged. `TryGetTagPathLiteral` already returns `false` cleanly on every non-match; the receiver
|
||||
guard is one more early `return false`. The method-match out-param is only read when the gate returns
|
||||
`true`.
|
||||
|
||||
## Testing
|
||||
|
||||
xUnit + Shouldly in the existing AdminUI test project, exercising `ScriptAnalysisService.Complete` /
|
||||
`Hover` directly (NO bUnit — these are service-level, not Razor):
|
||||
|
||||
- `ctx.GetTag("` → tag-path completions offered.
|
||||
- `foo.GetTag("` / `something.SetVirtualTag("` → tag-path completions NOT offered (receiver guard).
|
||||
- Hover on a `ctx.SetVirtualTag("…")` literal → markdown carries the single-tag-drop note.
|
||||
- Hover on a `ctx.GetTag("…")` literal → unchanged (no spurious note).
|
||||
|
||||
**Live `/run`** on the docker-dev rig (login-disabled; Chrome-driven against `http://localhost:9200`
|
||||
ScriptEdit page + inline vtag modal Monaco): confirm `ctx.GetTag("` shows completions, a non-`ctx`
|
||||
receiver does not, and the `SetVirtualTag` hover shows the truthful note.
|
||||
|
||||
## Deferred / out of scope
|
||||
|
||||
- "Authoritative `SetVirtualTag` write-target key" — moot (cascade engine dormant; production write
|
||||
is a no-op).
|
||||
- UNS browse-path as a non-inserted completion *detail* (the catalog's own documented discoverability
|
||||
follow-up) — not in this phase.
|
||||
- Wiring the cross-tag cascade engine into the host (the real fix for making `SetVirtualTag`
|
||||
functional) — a much larger, separate effort.
|
||||
|
||||
## Done =
|
||||
|
||||
Build clean + `dotnet test` (AdminUI) green + live `/run` confirms the three behaviors → merge to
|
||||
master + push.
|
||||
Reference in New Issue
Block a user