In addition to the unit tests, it would be good to add a few end-to-end tests testing the commands like regal lint with flags like --timeout, --config-file, etc.

We should allow pluggable reporters of the linter results. The default, and likely the only one to include in the first release, should be a "pretty" version, that uses anything (colors, emojis..) to make the output pleasing to the eye.

We'll later need something like a JSON reporter if we want to do editor integrations or whatnot, but that can wait until later.

Conftest provides a number of reporter implementations, so could be we could use some of that for insipiration.

A pretty common mistake in OPA is to mistype some attribute in the config, which OPA will happily swallow most of the times. It would be awesome if Regal would be able to lint OPA config files at some later point in time :)

We currently only check for references to "input" or "data", but not references to other rules, or data or input which might have been aliased.

It would be great if we had a linter rule to enforce correctness in metadata annotations. This rule would at minium check:

• No extra/unknown attributes present in the metadata YAML.
• Correct type provided for all attributes.

If we later allow for configuration of the linter, I could imagine quite a few rules to be configurable by the user, like ensuring that some attributes (title, description) are always present. But that can wait until later.

I believe we'll want some help from Go when reading metadata annotations. While we probably can do this in Rego alone, that solution is going to be quite complex, and it would be difficult to connect the metadata to the rules that they apply to... not to mention the whole metadata chain (if we want to work with that).

The reporter currently reports 1) the number of errors found and 2) the number of files in which there are errors.

A successful run results in the reporter coming back with "found 0 violations in 0 files", which makes it seem like the linter did not scan any files at all. We should report the above as we do, but add another number for the number of files scanned.

While we're at it, we should move the summary to the end of the output rather than having it at the top.

I got this idea from: https://github.com/orgs/open-policy-agent/discussions/334#discussioncomment-4845192

When new built-in functions are introduced, they sometimes "deprecate" custom built-in functions and other constructs used before them. Examples:

object_keys(o) := {k | _ = o[k[} # => object.keys
set1 & set2 == set2              # => object.subset
starts/endswith in loop          # => strings.any_prefix_match / strings.any_suffix_match
negation in loop                 # every

And I'm sure many more. Some of these will be easy, some very hard to identify correctly... but they could be tremendously useful for someone upgrading from an older version of OPA, or coming back to policy that hasn't been touched in a few years.

Another issue I found today with the "no external refs" for function bodies — the rule fails on something like:

f(x) := y {
   small := lower(x)
   "abc" == small
}

Since small is not provided in args. We'd need to include all vars defined in the body in this check.

As many other linters, Regal should support warning on lines longer than X number of characters.

120 seems like a good default, but this should be configurable.

Implicit assignment in boolean rules vs. explicit assignment using = looks the same in the parsed AST. These three constructs both render the same AST, which is perhaps not surprising given that they are "identical".

allow {
    true
}

allow = true {
    true
}

allow = true

All render the same AST for the rule:

{
    "head": {
        "name": "allow",
        "value": {"type": "boolean", "value": true},
        "ref": [{"type": "var", "value": "allow"}]
    },
    "body": [
        {
            "terms": {"type": "boolean", "value": true},
            "index": 0
        }
    ]
}

However, the second/third form should be considered a violation, as := should be used for assignment. Since we can't discern this using the AST alone, we'll need to investigate other methods, like scanning the unparsed code at that location (which should be possible with @charlieegan3's serialization fix in OPA) , extend the AST with more data, or create a custom function, or...

See discussion here: #37 (comment)

In order to know how the linter performs, and if there are certain groups of rules that could be improved, it would be useful if the linter collected and reported on metrics from evaluation. This could be included in the output if some verbose setting was toggled on, or a specific flag for metrics.

Similar to other linters, find occurences of TODO / FIXME comments and warn when found.

Users should be able to provide their own custom rules written in Rego. We could check for these in some well-known location, like .regal/rules in the project root directory.

Relates also to #20

This is currently done in two places right now. There's also too much data carried in the struct as the content is duplicated in both the FileContent item and in the AST.

Similar to how opa parse allows printing the AST of a single policy file, it would be useful to allow a regal parse command to print the entire "AST" created by regal, including all custom additions and data. This would make it easy to write and test custom rules using regular OPA.

For each rule, we should have documentation stating:

1. Example of the bad pattern
2. Example of a prefered pattern
3. Rationale + other notes
4. Possibly links to the Styra Academy and other resources

See e.g. the hadolint rule pages for examples: https://github.com/hadolint/hadolint#rules

Docs should be included in the repository in markdown format.

For later, obviously :) But it would be neat if Regal integrated with VS Code, either via the existing plugins, or new ones.

The Wasm POC @srenatus initiated would be a good place to start.

This could probably just call the formatter similarly to opa fmt --fail and warn if a file isn't properly formatted.

Using != in iteration is almost always a bug:

"admin" != input.user.roles[_]

What was likely intended:

not "admin" in input.user.roles

Add to new category named "bugs".

Testing Regal on some policy libraries found online, there are a lot of violations reported for style-related rules, like preferring := over = for top-level assignment. While we should be adamant about good style, there's also a clear risk that a user seeing hundreds of such violations either disables those rules, or worse — misses more severe violations as they drown in the amount of warnings reported.

For simple style rules like the one above, it would be interesting to explore if we could provide a remediation option, were we rewrote "bad" constructs to better ones without the user having to do the heavy lifting. Something similar to opa fmt, but limited to linter rules.

Since we have no concerns around backwards compatibility, I think we should import the future keywords in the evaluation context, making it unnecessary to do so in each linter policy. If we do, we'll want to document this too, of course.

While the print function is great for debugging, it's likely a mistake to leave it in before commiting. It would be good to have a check for that.

It would be good to provide a --timeout flag to allow the user to decide the threshold for evaluation. We don't know yet what running regal on a huge policy library would require, but I can imagine it to take some time, so we'll likely want to go with something like 30s for a default.

Avoid

input.number = 5

Prefer

input.number == 5

Motivation: while there are a few valid cases for unification, the unification operator (=) should not be used for simple comparison. TBD — more comprehensive explanation.

See: https://github.com/StyraInc/rego-style-guide#prefer-unconditional-assignment-in-rule-head-over-rule-body

Since we provide custom built-in functions, and later on, custom data to help with linting, users can't rely on opa test to test their custom rules. To help with that, we should provide a regal test command, which would work similarly to opa test but with our custom additions included.

The first iteration of the executable currently works with a single file only. We should improve this and allow any number of files and directories to be scanned. Running the entire set of rules against the AST of each individual Rego file is likely going to be quite expensive, and later we'll want to do some pre-processing of the files to create new datastructures more easily traversed (#13), but we can start with a naive implementation and just create an array of all the files (or rather, the AST representation of each), and scan them one-by-one (possibly in paralllell).

A rule configured with severity "warning" should be included in the report, but should not fail the "build", i.e. not by itself change the exit code to a non-zero value.

To start with something, we should build a simple Go application that embeds all the rules in the policy dir (excluding tests). As a dogfooding excercise, the linter could be used to lint the linter policies themselves in a GHA pipeline.

Ignoring "complexity" for now, it would be good if Regal could at least flag rules or functions with a body exceeding X number of lines. The default could be something liberal, like 20(?), but this should be configurable.

Perphaps we should ignore lines and just count expressions.

While we could do this by hand, this is likely to get out of sync with the real world quickly. It should be fairly "easy" to create a Go utility, that scans metadata of all rules in the bundle directory and creates a markdown table for display. This table should include:

• Category
• Rule
• Description
• Enabled (by default)

The rule name should be a link pointing to the Styra docs for the rule.

foo {
	some x
	input[x]
}

Even in strict mode, OPA considers x to be "used", as it's referenced somewhere. Howver, unless x is referenced later in the rule (or in the rule head) it's not really used, and the above could be replaced by:

foo {
	input[_]
}

Or

foo {
	some _ in input
}

Since we'll be depending on OPA anyway, it would be useful to include all of the strictness checks from OPA but in the form of linter checks. These would naturally not be described in Rego, but would be displayed as other linter violations without requiring the use of another command (opa check, opa eval, ...) and would have links back to our documentation portal like the other linter rules do.

We should allow providing a configuration file for enabling/disabling rules, as well as more fine-grained controls for rules where that's applicable.

I think it would make sense to use a .regal directory, where the user could provide both the configuration file, as well as custom Rego rules.

While OPA allows it, using multiple test rules with the same name makes it impossible to know what a test actually tests just by reading its name. Using distinct names for each test should be preferred.

Note: should ideally be checked at package level rather than file, but we currently scan one module at a time. Perhaps we could start with what we have.

This came up in a discussion yesterday. Warn when # METADATA is placed anywhere but on top of packages or rules.

allow {
    1 == 1
    true
    false
}

And so on...

Note that we can't do much about a single true in an otherwise empty body, as:

allow := true

will be rewritten as:

allow = true {
    true
}

So we can accept that as an exception for now.

All rules supplied to Regal should have the following set:

• title
• description
• custom.category

x-ref: #37 (comment)

Avoid

"foo" == input.bar[_]

Prefer

"foo" in input.bar

Motivation: easier to read, makes it just as easy to invert the check using not which isn't possible using the iteration construct.

Will the below policy work or not?

package play

f(arr) := upper(arr[_])

x := f(input.arr)

The answer depends on the number of elements in input.arr. 1 item is fine, more than that and you'll get:

policy.rego:3: eval_conflict_error: functions must not produce multiple outputs for same inputs

The big problem with this is that you can test, publish and run a policy for days or longer, before the problem manifests at runtime. The compiler accepts this, but it would be extremly helpful if Regal didn't.

For both our own use, as well as for authors of custom rules, it would be really useful if we had a JSON schema defining the AST JSON, including any custom additions we have added. This would help catch mistakes in Regal policy, and could be integrated as part of our build pipeline.

One thing that would be interesting (and potentially useful) is if Regal included some optional rules to check the directory structure of a policy repo, and reported on things that could be improved:

• corresponding *_test.rego for any rego file
• or a separate test directory
• file names match package names
• .editorconfig
• "bundle mode" checking for .manifest

And so on... I'm sure we could come up with more. Low priority, but noting it down for later :)

For the purpose of getting started, our rules currently leverage a (perhaps too) simple model of the input, where each linter rule iterate over rules, expressions and terms looking for whatever is relevant for the rule to enforce. This however means that each rule traverses almost the entire AST. While I don't think performance is a top concern at the moment, it probaly could be later when used in larger projects. A more immediate concern however, is how the "flat" iteration model will miss some significant parts of the AST, anytime another level of nesting is encountered. There are at least two (and possibly more I haven't thought of?) types of constructs where this happens:

• comprehensions
• the body of an every statement

Both of these create a new nested context, which in turn of course could contain even more nested comrehensions or bodies. While I don't think we'll need to worry too much about supporting "infinte" levels of nesting — as anything more than 2-3 levels would arguably be a code style violation itself, we'll definitely will want the linter to cover this code! How? Without recursion, this is non-trivial. Some options I can think of:

1. Each rule use something like the walk builtin to traverse the AST — even more expensive.
2. Have each rule declare its "dependencies" — i.e. the type of constructs that the rule applies to, like "function calls", "expressions", "imports" and so on... and Regal would prepare a "view" of the input based on that.
3. Have regal run an "indexing" step before executing any rules, where the AST is traversed and groups are created for each type of construct, which the rules could then make use of.
4. Maybe have Go code (via a custom builtin function or whatnot) provide a way to "flatten" nested constructs, or by other means allow linter rules to keep with the "simple" model, but have a tool to help with nesting. I'm not sure what that would look like though.

I think option 3 is appealing, as it would allow rules to deal only with the constructs that are relevant to them, and without having to know much about the context in which they're found. But we'd need to give this some more thought.

While we'll want to enforce snake_case naming of rules and vars by default, it'd be nice if we provided an optional rule to enforce custom naming conventions. This could be provided in the form of a regex, that would be checked against all rule names and vars.

As pointed out by @charlieegan3, we could check for more types than string + var in the "prefer in over iteration" rule.

Avoid

x = 5

Prefer

x := 5

Motivation: while there are a few valid cases for unification, the unification operator (=) should not be used for simple assignment. TBD — more comprehensive explanation.

I noticed this today: open-policy-agent/opa#2598

And while it could potentially be done in OPA, this seems like an excellent use-case for Regal. The example rules mentioned in the ticket:

• App repos should not be able to modify the system package except for the system/log/mask decision
• App policy packages must be namespaced under package acmecorp.<app_name>
• App API authorization policies must include a default allow = false rule (any other value is not allowed for the default allow rule)

All seem like they would be quite easy to add as optional, configurable rules. If we want to leave it outside of Regal core, I could see how we could provide these type of rules in an external bundle... but having them packaged would be convenient.

Calling a built-in function returning a non-boolean value without actualy assigning or using that value is likely a mistake.

Not sure about rules, but perhaps we'd want to check those too 🤔

I've seen a few policies where expressions are evaluated in each iteration of a loop even though they don't make use of values bound in the loop, i.e.

allow {
    some user in data.users
    endswith(input.email, "acmecorp.com") # this should be moved out of the loop
    # more things here
}

Warning on this would be great.