Extract shared constants into package constants.tql - #163
Merged
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
jachris
force-pushed
the
topic/extract-package-lets
branch
from
June 24, 2026 12:46
03171ae to
399455b
Compare
jachris
changed the title
lets.tqlconstants.tql
This was referenced Jun 24, 2026
jachris
force-pushed
the
topic/extract-package-lets
branch
from
June 24, 2026 13:20
399455b to
d1b8b28
Compare
Member
|
I'm surprised there are so few packages we are extracting constants from. Have you tried Codex for the same task once? |
Contributor
Author
My instructions were that it should only extract shared constants. If something is only used locally, I think it makes sense to stay there. Or do you see anything that would still be worthwhile to refactor? (Also, feel free to push to this RP if you want) |
Member
|
All good, i just wondered if you went through all packages or hand-picked a few. I trust that this is complete for what we have. |
jachris
added a commit
to tenzir/tenzir
that referenced
this pull request
Jun 26, 2026
## 🔍 Problem Packages bundle operators, pipelines, and contexts, but offered no way to define shared named constants. Authors had to repeat literal values across a package's operators and pipelines. ## 🛠️ Solution A package can define `let` bindings in a `lets.tql` file at its root. Each binding is a compile-time constant, referenceable as `pkg::$name` — from the package's own operators and pipelines, and from any external pipeline that uses the package. Later bindings may reference earlier ones. ```tql // acme/lets.tql let $high_severity = 8 let $threshold = $high_severity + 1 ``` ```tql where severity >= acme::$threshold ``` References are resolved and const-evaluated lazily at each use site, where the full registry is available — mirroring how user-defined operators resolve. A binding must be a deterministic constant; `let $r = random()` is rejected, since per-reference evaluation would otherwise yield differing values. ## 💬 Review - New `pkg_dollar_var` AST node and `let` entity namespace; references resolve against the `packages` domain and cache their constant on the node. - Bindings are stored unevaluated at load and const-evaluated lazily on first reference, so sibling and cross-package references resolve against a complete registry. - Operators and lets register through a shared `build_package_module`. - `expression::is_deterministic` is made exact and exhaustive so composite constants (lists, records, …) are no longer misclassified; it gates the determinism check. - Tests cover internal and external references, sibling references, and the error paths (unknown binding, unknown package, forward reference, non-`let` statement in `lets.tql`, reference cycle, non-constant and non-deterministic bindings). <sub> 📎 Plugins PR: tenzir/tenzir-plugins#568<br> 📚 Docs PR: tenzir/docs#413<br> 📎 Related: tenzir/library#163 </sub>
Several OCSF mappers copy-pasted the same constant lookup maps (hash algorithm ids, signature states, severity/status/verdict enums, ...) across many operator files. Hoist the genuinely duplicated, constant ones into a per-package `constants.tql` and reference them as `pkg::$name`, the new package-level `let` bindings. Mappers that use a differently-shaped map of the same name (e.g. Okta's extended ALLOW/DENY status map, Microsoft's Defender alert status, the per-class FortiGate disposition vocabularies) keep their local `let`. No behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The CIFS and SSH mappers had a two-entry disposition map (blocked, allowed) while the sibling UTM content-inspection mappers (webfilter, emailfilter, virus) also map `monitored` to OCSF disposition_id 17. A `monitored` action on a CIFS/SSH log therefore fell through to 0 (Unknown). Route both through the shared `fortinet::$utm_dispositions` binding, which includes `monitored`, fixing the silent misclassification. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
jachris
force-pushed
the
topic/extract-package-lets
branch
from
August 6, 2026 14:53
d1b8b28 to
fdd3067
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🔍 Problem
OCSF mappers across several library packages copy-paste the same constant
lookup tables (hash-algorithm IDs, severity/status/verdict enums, FortiGate
dispositions). The copies drift: FortiGate CIFS/SSH omit the
monitoreddisposition their sibling mappers have, so a
monitoredaction silently mapsto
0(Unknown) instead of17.🛠️ Solution
Hoist the genuinely duplicated constant maps into a per-package
constants.tqland reference them as
pkg::$name(the new packageletbindings).constants.tqladded to sysmon, microsoft, okta, fortinet, and sophos.let.fortinet::$utm_dispositionsalso fixes the missing
monitoredmapping (separate commit + changelog).💬 Review
- No behavior change apart from the FortiGate CIFS/SSH
- Depends on the engine PR shipping
📚 Docs PR: tenzir/docs#413monitoredfix.tenzir-testagainst a local build: sysmon 30/30, fortinet 1/1, okta 7/7,sophos 7/7, microsoft 74/75 — the one failure is a pre-existing, unrelated
ASIM test (reproduces on a clean tree).
constants.tql; library CI runs againstreleased Tenzir via
uvx, so it stays red until that release.📎 Related: tenzir/tenzir#6363