refactor(nse): migrate to shared go-api/store/templates package (PR 7) - #3
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bf96d0fdf5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| script, exists := manifest.Scripts[canonicalID] | ||
| if !exists { | ||
| script, exists = manifest.Scripts[scriptID] | ||
| } |
There was a problem hiding this comment.
UpdateScriptFromUI claims to accept both canonical IDs and legacy extension IDs, but the lookup only tries manifest.Scripts[canonicalID] and manifest.Scripts[scriptID]. When the on-disk manifest still uses <name>.nse keys (as your new tests describe), a canonical UI ID like acarsd-info misses both entries and returns "script not found", so valid scripts cannot be edited. Add a fallback for the legacy .nse key (or normalize manifest keys before lookup).
Useful? React with 👍 / 👎.
| canonicalID := templates.CanonicalScriptID(id) | ||
|
|
||
| // Get script content from ValKey first (highest priority) | ||
| globalContent, err := sm.getScriptContent(context.Background(), id) | ||
| globalContent, err := sm.getScriptContent(context.Background(), canonicalID) |
There was a problem hiding this comment.
syncScriptContent now reads script data only via the canonical key (templates.NseScriptKey(canonicalID)) and never checks the legacy nse:script:<id>.nse entries written by prior versions. During upgrade, existing edited content in old keys is treated as missing, then the local file is written to the new key, which effectively drops user edits from the active path. A legacy-key fallback/migration read is needed before writing canonical keys.
Useful? React with 👍 / 👎.
Bump go-api to v0.0.18 and replace the local CanonicalScriptID helper plus the hand-rolled key prefixes with the shared package constants and helpers. Re-export the key constants from internal/nse/types.go as aliases so existing call sites compile unchanged. Wire the script-content writer through templates.WriteNseScript so canonicalization is enforced in one place.
1aad183 to
3d3cd24
Compare
1 participant
Summary
Part of the scanner-templates-fix sprint (PR 7 / 8). Bumps go-api to v0.0.18 (which introduced `sirius/store/templates` in PR 6) and replaces the local NSE canonicalization helper + key string literals with the shared package's helpers.
No wire-format change. The shared package's NseScriptRecord is byte-compatible with the existing local ScriptContent envelope (same JSON tags), so previously-written `nse:script:*` entries decode unchanged.
Test plan