Skip to content

:string:starts_with: evaluate the pattern argument - #93

Closed
brian-slashguard wants to merge 1 commit into
google:mainfrom
slashguard:upstream-startswith
Closed

brian-slashguard wants to merge 1 commit into
google:mainfrom
slashguard:upstream-startswith

Conversation

@brian-slashguard

Copy link
Copy Markdown

:string:starts_with evaluates its first argument through the substitution but takes its second raw:

pat, ok := pattern.Args[1].(ast.Constant)   // raw
str, ok := evaluatedArg.(ast.Constant)      // evaluated

So the pattern must be a string literal written into the program text. A prefix computed from data is rejected:

resolves(T, F) :- activation(_, T), control(_, F, _),
                  P = fn:string:concat(T, "/"), :string:starts_with(F, P).

That makes a whole class of prefix join inexpressible — matching an unversioned identifier against versioned ones is the case we hit.

This evaluates the second argument the same way the first already is. Literal patterns are unaffected; computed ones now work.

Verified: with activation tokens soc2 and pci-dss against frameworks soc2/v0.1 and pci-dss/v0.1, resolves/2 yields both pairs, and a token with no matching framework correctly yields nothing. go build ./... is clean.

:string:ends_with and :match_prefix have the same asymmetry — happy to extend this to them if you'd like it done consistently.

starts_with evaluates its first argument through the substitution but takes its second
raw, so the pattern must be a string literal written into the program text. A prefix
computed from data — fn:string:concat(Token, "/") — is rejected, which makes a common
class of prefix join inexpressible.

Evaluates the second argument the same way the first already is. Literal patterns are
unaffected; computed ones now work.

Verified against a versioned-identifier join: with activation tokens 'soc2' and
'pci-dss' and frameworks 'soc2/v0.1' and 'pci-dss/v0.1', resolves/2 yields both pairs,
while a token with no matching framework correctly yields nothing.
@google-cla

google-cla Bot commented Aug 2, 2026

Copy link
Copy Markdown

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@burakemir

Copy link
Copy Markdown
Contributor

It should not be necessary to change the implementation in builtin: the engine should evaluate expressions like :string:starts_with (replacing variables with the values they are bound to). So if you have a computed pattern, it should already work.

There is also analysis that ensures that both arguments to :string:starts_with are inputs (mode check).

Do you have an example of a rule with a computed pattern that does not work?

One thing that I noticed which is inconsistent in the existing code is that the builtin code re-evaluates arg[0] while it does not need to. However, if we want to fix that, we should fix that in a way that it consistently removes the needless evaluation.

@brian-slashguard

Copy link
Copy Markdown
Author

Withdrawing this. You were right to ask for a reproducible failure, and I could not produce one.

Retested against unpatched upstream at the commit this PR targets, the rule this was raised for
works unchanged:

resolves(T,F) :-
  activation(_,T), control(_,F,_),
  P = fn:string:concat(T,"/"),
  :string:starts_with(F,P).

Both expected matches are returned and the unmatched token is excluded, through the interpreter and
through our own evaluation pipeline. Your point about the engine evaluating both arguments before the
builtin is called is the explanation: the computed prefix has already become a constant by then. The
builtin does read asymmetrically — one argument evaluated, the other inspected directly — and that
is what led me to misread it as a missing capability, but confusing code is not a defect and I should
have established the failure before proposing the fix.

Checking our own corpus afterwards, the pattern this PR exists to enable is used in none of our
rules: every variable prefix we pass to :string:starts_with is bound from a fact rather than
computed, so the capability was never load-bearing for us either.

The process failure was mine and it is worth naming: "works after the patch" was never shown to be
"fails before the patch", and neither of my PRs carried a regression test that would have exposed
that. Thank you for pushing back rather than merging it.

If a regression test for computed prefixes would be useful to you, I am happy to send one on its own.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants