Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
1 task done
osheari1 opened this issue Nov 29, 2024 · 8 comments
Open
1 task done
Labels
bug Something isn't working

Comments

@osheari1
Copy link

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

  • Yes I was.

Relevant log output

No response

Relevant screenshots (optional)

No response

@osheari1 osheari1 added the bug Something isn't working label Nov 29, 2024
@mattjoyce
Copy link
Contributor

mattjoyce commented Dec 1, 2024

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
Copy link
Author

osheari1 commented Dec 3, 2024

Hello!

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

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
Copy link
Contributor

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

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

@osheari1
Copy link
Author

osheari1 commented Dec 4, 2024

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

There are two cases where this issue could pop up:

  1. when we have jekyll vars (as an example) in a prompt
    This would still be an issue with '--novars'.
  2. 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
Copy link
Contributor

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

@osheari1
Copy link
Author

osheari1 commented Dec 5, 2024

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
Copy link
Contributor

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.

@mattjoyce
Copy link
Contributor

Solved with #1189

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants