Repository navigation
Conversation
|
The following comment was made by an LLM, it may be inaccurate: Potential Duplicate Found:
Why they're related: Both PRs address the same issue (#5054) and implement the exact same feature: |
There was a problem hiding this comment.
Pull request overview
This pull request adds environment variable substitution functionality to markdown-based agent configuration parsing. Users can now use the {env:VAR_NAME} syntax in both frontmatter and prompt content of markdown agent files, with unset variables being replaced with empty strings.
Changes:
- Added
substituteEnv()function to perform environment variable replacement using{env:VAR}syntax - Integrated environment variable substitution into the markdown parsing pipeline
- Added comprehensive test coverage for the new functionality including edge cases
- Updated documentation with usage examples and security guidance
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/opencode/src/config/markdown.ts | Implements the substituteEnv function and integrates it into the parse pipeline |
| packages/opencode/test/config/markdown.test.ts | Adds comprehensive test suite with proper environment variable setup/teardown |
| packages/opencode/test/config/fixtures/env-frontmatter.md | Test fixture demonstrating env var substitution in both frontmatter and content |
| packages/web/src/content/docs/agents.mdx | Documents the new feature with usage examples and security considerations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
f7bf935 to
b4be2bf
Compare
|
After consultation with a couple of models and further review of the code changes, I do believe that I have come to the conclusion that my take in #5092 does adhere more closely to the original requestor of the Issue's request in #5054 to provide replacement only in the YAML/frontmatter. The version here does expand the scope slightly by providing replacement throughout the body as well. Whether this expansion in scope is desirable or not I'll leave to the maintainers to decide as I don't hold any strong opinion on that aspect myself. In any case, either PR seems that it would give the original requestor the ability to do what they wanted to do surely the resolution of their issue would provide them with a pleasant surprise - and if they're happy, so am I, so hopefully one of these PRs will come in to a safe landing. |
b4be2bf to
db8a8c2
Compare
|
Thanks your review, I update the document with my real usage, I need ENV in markdown body, so hoping this PR can be choosen. (otherwise, I have to write the prompt in jsonc....) |
db8a8c2 to
fbccf01
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| describe("ConfigMarkdown: env substitution", () => { | ||
| const tokenKey = "TEST_MCP_TOKEN" | ||
| const emptyKey = "EMPTY_ENV_VAR" | ||
| const prevToken = process.env[tokenKey] | ||
| const prevEmpty = process.env[emptyKey] | ||
| let parsed: Awaited<ReturnType<typeof ConfigMarkdown.parse>> | ||
|
|
||
| beforeAll(async () => { | ||
| process.env[tokenKey] = "abc123" | ||
| delete process.env[emptyKey] | ||
| parsed = await ConfigMarkdown.parse(import.meta.dir + "/fixtures/env-frontmatter.md") | ||
| }) | ||
|
|
||
| afterAll(() => { | ||
| if (prevToken === undefined) delete process.env[tokenKey] | ||
| else process.env[tokenKey] = prevToken | ||
| if (prevEmpty === undefined) delete process.env[emptyKey] | ||
| else process.env[emptyKey] = prevEmpty | ||
| }) | ||
|
|
||
| test("should substitute env vars in frontmatter", () => { | ||
| expect(parsed.data.description).toBe("Token is abc123") | ||
| }) | ||
|
|
||
| test("should substitute missing env vars with empty string in frontmatter", () => { | ||
| expect(parsed.data.note).toBe("") | ||
| }) | ||
|
|
||
| test("should substitute env vars in content", () => { | ||
| expect(parsed.content).toContain("Token: abc123") | ||
| expect(parsed.content).toContain("Missing: ") | ||
| }) | ||
| }) |
There was a problem hiding this comment.
The test only verifies that env variables are replaced with their values or empty strings, but doesn't test edge cases like nested braces, malformed patterns, or special characters in variable names. Consider adding test cases for: 1) Multiple substitutions in the same line, 2) Variables with underscores and numbers, 3) Malformed patterns like {env:} or {env:VAR with spaces}, 4) Variables that contain special regex characters.
| ```markdown title="~/.config/opencode/agents/mcdonalds.md" | ||
| --- | ||
| name: "McDonalds" | ||
| description: Agent to help you ordering McDonalds |
There was a problem hiding this comment.
The description contains a grammatical error. It should be "Agent to help you order from McDonalds" instead of "Agent to help you ordering McDonalds".
| description: Agent to help you ordering McDonalds | |
| description: Agent to help you order from McDonalds |
|
@Eric-Guo I expect that surely you must already be aware, but just in case: you can of course already achieve your terminal goal of using environment variable in the body by using shell expansions. It's not a markedly efficient way to do it, of course, since we're starting a whole shell just to get at an environment variable, but it does work, so it could perhaps suffice in the meantime. Best of luck! |
How to do that? I don't know it can, seems not mention in document. It sounds useful but more crazy and less security (of course)... But if I can do that, this PR is no need. 😊 |
|
You would do what is shown here: https://opencode.ai/docs/commands/#shell-output Simply use a command such as 
As stated previously, it's probably not the most efficient way to do it, but it should work. |
c675b6e to
39dcd7b
Compare
|
What I known is command is need to trigger by user, anyway to let the command being triggerd automatically at starting, without user need to do any action? i.e. system prompt can calling command directly? If answer is no, it's more my need.... |
|
The commands in the shell expansions run each time you run the slash command. This is in most cases beneficial: after all, if you run the |
6d02317 to
2fcd86e
Compare
d5e033a to
9cfdf94
Compare
4cc021c to
becedb8
Compare
becedb8 to
60905d0
Compare
00637c0 to
71e0ba2
Compare
f1ae801 to
08fa7f7
Compare
60533cf to
35211b1
Compare
45f2d67 to
a925782
Compare
e986e86 to
2a941b7
Compare
2a941b7 to
949677d
Compare
586653b to
ec4b24b
Compare
5ec17c6 to
f3f97e6
Compare
…nt definitions in .md now get env values injected before frontmatter/prompt parsing.
f3f97e6 to
d861162
Compare
|
Automated PR Cleanup Thank you for contributing to opencode. Due to the high volume of PRs from users and AI agents, we periodically close older PRs using automated criteria so maintainers can focus review time on the most active and community-supported contributions. This PR was closed because it matched the following cleanup criteria:
PRs created within the last month are not affected by this cleanup. If you believe this PR was closed incorrectly, or if you are still actively working on it, please leave a comment explaining why it should be reopened. A maintainer can review and reopen it if appropriate. Thanks again for taking the time to contribute. |
Close #5054
The key difference between this PR and #5092 is this PR allow ENV usage in markdown body.
I already including a real use case in document and give the warning of security concerns.
Notice current json configuration of prompt allow using ENV, so I think using ENV in markdown body should also allowed.
Other coding agent like kimi-cli using ENV in their system prompt, so security concerns is not as serious as the first sight.