[Bug]: Running `improve_prompt` on a prompt with template variables fails

osheari1 commented 1 week ago

What happened?

With a prompt, such as improve_security_policy

# IDENTITY and PURPOSE
You are an expert in cybersecurity standards and policies. You specialize in creating clear, well written policies for companies and corporations (for example for certifications such as SOC2 and HiTrust).

You have been tasked with improving an existing security policy that needs to be improved, and you must output an improved report finding in markdown format.

# GOAL
Given a policy type and set of requirements, provide a well written, concise improved security policy in markdown format.

# STEPS
...
# OUTPUT INSTRUCTIONS
...
# INPUT
INPUT:
## Requirements: 
{{reqs}}
## Policy

piping into improve_prompt returns

$ cat system.md | fabric --pattern improve_prompt 
missing required variable: reqs

Expected behavior: Improve prompt would work on patters with variables / plugins / other future stuff that may be added to the templating features.

I'm assuming this is because variables are validated after stdin is substituted into the template.

Version check

[X] Yes I was.

Relevant log output

No response

Relevant screenshots (optional)

No response

mattjoyce commented 1 week ago

hello @osheari1 yes, you're correct.

the user input and pattern are combined an then the template system tries to resolve all double brace pairs. {{ }} A work around might be just to provide a variable. -v=req:""

potentially we could have --novars or something to disable substitution.

osheari1 commented 4 days ago

Hello!

I'm wondering if in the long run, a more robust solution would be needed... :thinking:

Thinking of two cases:

Prompts or INPUT that contains non-fabric variable substitution {{...}} syntax (eg. Jekyll and other template libs),
INPUT that contains fabric variable substitutions that we don't want substituted

Pattern

# Example Output
// Some example explaining Jekyll syntax
{{ jekyll-var-no-sub-wanted }}

# Output 
{{ some-fabric-variable }}

Input

Stuff {{ fabric-var-no-sub-wanted }}

Here, {{ fabric-var-no-sub-wanted}} and {{ jekyll-var-no-sub-wanted }} would both throw errors.

Problem

All {{...}} syntax in prompt and input are substitution targets. Aka fabric doesn't know which variable should require substitution.
- To users of a very complex prompt with multiple non-fabric and fabric var substitutions, it may get confusing as to which substitutions you actually want to substitute.
Variable substitution happens on both prompt and input at the same time.

Questions

Do we ever want to have variable substitution handled by fabric for INPUT? Given we are piping raw input into fabric, couldn't the substitution for input be handled more effectively outside of fabric (from an architecture standpoint)? Having piped input substituted unknowingly seems like an unnecessary side effect.

For example, say you have a pipeline, cat ... | fabric ... | command A | fabric ... | command B | fabric ...: Depending on the task, you won't necessarily know what output the fabric commands will produce. Meaning, in some cases, they may produce instances of {{...}} in intermediary steps, which will then, without the users knowledge, attempt to be substituted by fabric in later calls.

For example,

$ cat stuff.txt | sed 's/{{var}}/subbed/g' | fabric -p do_stuff

Solutions?

Apply -v=req:"{{jekyll-variable-no-substitution-wanted}}": This could work, but its not very robust nor user friendly.
Run variable substitution on the pattern before combining with INPUT. This would prevent accidental substitution.
Syntax for substitution could attempt to be unique and / or require escaping. Eg for Jekyll \{\{ escaped-jekyll \}\}.

mattjoyce commented 3 days ago

I'll admit, I did not consider folk would want to process jekyll templates when I made the template system.
I do use variables in input, it useful and think this will be increasingly useful when extension are available. Probably the simplest approaches to this are

Do not fail if a variable is missing - perhaps a .env setting to disable or enable.
Include a cli option to not process vars. --skiptemplate, --novars or similar.

osheari1 commented 3 days ago

Yeah I think that will work if it applies specifically to INPUT vars.

There are two cases where this issue could pop up:

when we have jekyll vars (as an example) in a prompt This would still be an issue with '--novars'.
when we have jekyll or fabric vars in input This would be solved by --novars, if --novars only applies to INPUT vars only.

Is there something that could be done about the first case as well?

mattjoyce commented 2 days ago

Honestly I think it's just easier to not fail on missing vars.

osheari1 commented 2 days ago

I think that would be helpful. For this case, what if we added some debugging information about variable substitution when '--dry-run' (or some '--debug' flag) is passed? If we skip var validation entirely, people may unexpectedly not pass -v=<name>:<val> when they should.

Perhaps something like (pseudo-code)

$ echo "<user> | fabric --dry-run/debug --pattern <pat> -v=name-1:val-1 -v=name-2:val-2
...
variable,value
name-1,val-2
name-2,val-2
# name 3 was in prompt/input but not passed
name-3, <missing>

mattjoyce commented 1 day ago

It would be useful to have --debug, I think that is one 1st class option that is missing. I included it in template, but you have to recompile, it would be good to expose it to cli.

danielmiessler / fabric