feat: add file:// URI support in agent prompt_append#821
feat: add file:// URI support in agent prompt_append#821devxoul wants to merge 4 commits intocode-yeongyu:devfrom
Conversation
|
@sisyphus-dev-ai review this ulw |
sisyphus-dev-ai
added
sisyphus: working
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 4 files
Confidence score: 3/5
- Handling of percent-encoded file URIs in
src/agents/utils.tsis incomplete, so validfile://paths with encoded characters won’t load and instead produce false warnings. - This impacts users relying on encoded characters in file paths, making URI handling less reliable than expected.
- Pay close attention to
src/agents/utils.ts- ensure percent-encoded segments are properly decoded before path resolution.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/agents/utils.ts">
<violation number="1" location="src/agents/utils.ts:128">
P2: File URI parsing ignores percent-encoding, so valid file:// URIs with encoded characters won’t resolve and trigger false warnings instead of loading the file.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
sisyphus-dev-ai
added
the
sisyphus: working
Ultrawork Review: PR #821 - file:// URI Support in prompt_appendOverall Assessment: ✅ Good feature with VALID bug identified by cubic-dev-ai ✅ What's Excellent
🐛 VALID Issue (identified by @cubic-dev-ai)The percent-encoding issue is REAL and should be fixed. ProblemCurrent implementation strips const path = prompt_append.slice(7) // Just removes "file://"
const abs = path.startsWith("~/") ? resolve(homedir(), path.slice(2)) : ...This DOES NOT handle percent-encoded URIs:
Evidence from CodebaseThe project already uses the standard solution elsewhere:
Recommended FixUse Node.js built-in import { fileURLToPath } from "node:url"
if (prompt_append.startsWith("file://")) {
try {
const abs = fileURLToPath(prompt_append) // Handles percent-encoding + all edge cases
resolved = existsSync(abs)
? readFileSync(abs, "utf-8")
: `[WARNING: Could not resolve file URI: ${prompt_append}]`
} catch (err) {
resolved = `[WARNING: Invalid file URI: ${prompt_append}]`
}
}Why this is better:
📝 Missing Test CaseAdd test for percent-encoded URIs: test("resolves file:// URI with percent-encoded spaces", () => {
// #given - file with spaces in name
const fileWithSpaces = join(TEST_DIR, "custom prompt.md")
writeFileSync(fileWithSpaces, "Content from spaced file")
const encodedPath = fileWithSpaces.replace(/ /g, "%20")
const overrides = { oracle: { prompt_append: `file://${encodedPath}` } }
// #when
const agents = createBuiltinAgents([], overrides, TEST_DIR)
// #then
expect(agents.oracle.prompt).toContain("Content from spaced file")
})🎯 Verification Checklist
🚀 RecommendationACTION REQUIRED: Fix the percent-encoding bug before merge. The feature is solid but the bug will cause frustration for users with:
Using Review completed with exhaustive search across:
Credit to @cubic-dev-ai for identifying this - the issue is accurate and should be addressed. |
|
The flaky test failure in Fixed in #822 |
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/agents/utils.ts">
<violation number="1" location="src/agents/utils.ts:128">
P2: Uncaught decodeURIComponent URIError can crash agent config creation on malformed file:// paths.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
sisyphus-dev-ai
removed
the
sisyphus: working
Fix Applied: Percent-Encoding SupportThanks @cubic-dev-ai for identifying this issue and @sisyphus-dev-ai for the detailed review! What was fixedCommit const path = decodeURIComponent(prompt_append.slice(7))Test case added: Verifies that Why
|
| URI Format | fileURLToPath() |
decodeURIComponent() |
|---|---|---|
file:///absolute/path |
✅ Works | ✅ Works |
file://~/home/path |
❌ ERR_INVALID_FILE_URL_HOST |
✅ Works |
file://./relative/path |
❌ ERR_INVALID_FILE_URL_HOST |
✅ Works |
This PR intentionally introduces custom URI formats (file://~/... and file://./...) that extend standard file URIs for user convenience. These are not valid RFC 8089 URIs, so fileURLToPath() rejects them.
The decodeURIComponent() approach:
- ✅ Handles percent-encoding (spaces, special chars, non-ASCII)
- ✅ Preserves home-relative path support (
~/) - ✅ Preserves relative path support
- ✅ All tests pass
|
@sisyphus-dev-ai review again ulw |
sisyphus-dev-ai
added
the
sisyphus: working
Ultrawork Review: PR #821 - file:// URI Support (Round 2)Overall Assessment: ✅ What's Working Excellently
🐛 CRITICAL BUG (identified by @cubic-dev-ai)Issue: Evidence (empirically verified): decodeURIComponent('test%20file.md') // ✅ Works: "test file.md"
decodeURIComponent('test%') // ❌ Throws: URIError: URI error
decodeURIComponent('test%GG') // ❌ Throws: URIError: URI errorVerified with: bun --eval "console.log(decodeURIComponent('test%20file.md')); try { decodeURIComponent('test%'); } catch(e) { console.log('CRASH:', e.message); }"
# Output:
# test file.md
# CRASH: URI errorImpact:
Real-world scenarios:
💡 Recommended FixApply @cubic-dev-ai's suggested fix with minor refinement: if (prompt_append.startsWith("file://")) {
const uriPath = prompt_append.slice(7)
let path: string
try {
path = decodeURIComponent(uriPath)
} catch (e) {
// Malformed percent-encoding - show warning and skip file resolution
resolved = `[WARNING: Malformed file URI (invalid percent-encoding): ${prompt_append}]`
path = "" // Skip file resolution
}
if (path) {
const abs = path.startsWith("~/")
? resolve(homedir(), path.slice(2))
: isAbsolute(path)
? path
: resolve(configDir ?? process.cwd(), path)
resolved = existsSync(abs)
? readFileSync(abs, "utf-8")
: `[WARNING: Could not resolve file URI: ${prompt_append}]`
}
}Why this approach:
📝 Missing Test CaseAdd test for malformed percent-encoding: test("handles malformed percent-encoding in file URI with warning", () => {
// #given - malformed percent-encoding
const overrides = {
oracle: { prompt_append: "file://my%prompt.md" },
}
// #when
const agents = createBuiltinAgents([], overrides, TEST_DIR)
// #then
expect(agents.oracle.prompt).toContain("[WARNING: Malformed file URI (invalid percent-encoding): file://my%prompt.md]")
})🎯 Verification Checklist
🚀 RecommendationACTION REQUIRED: Add try-catch around This is a 5-minute fix that prevents potential session crashes. The feature is otherwise production-ready and well-designed. 🙏 Credit
Review Methodology:
The bug is real, the fix is simple, and the feature will be rock-solid after this final change. Confidence: 100% - Bug is empirically verified, fix is standard practice, and the rest of the implementation is excellent. |
sisyphus-dev-ai
removed
the
sisyphus: working
Fix Applied: Malformed URI Error HandlingThanks @sisyphus-dev-ai for the thorough review! What was fixedCommit let path: string
try {
path = decodeURIComponent(prompt_append.slice(7))
} catch {
resolved = `[WARNING: Malformed file URI (invalid percent-encoding): ${prompt_append}]`
path = ""
}Test case added: Verifies that Verification
|
There was a problem hiding this comment.
2 issues found across 2 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/agents/utils.ts">
<violation number="1" location="src/agents/utils.ts:137">
P1: `file://` prompt_append reads arbitrary filesystem paths (including absolute/home) without sandboxing, enabling secret exfiltration by untrusted config</violation>
<violation number="2" location="src/agents/utils.ts:142">
P2: Uncaught fs errors on file:// prompt_append can crash config merge (invalid path or unreadable file throws before warning)</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| } | ||
|
|
||
| if (path) { | ||
| const abs = path.startsWith("~/") |
There was a problem hiding this comment.
P1: file:// prompt_append reads arbitrary filesystem paths (including absolute/home) without sandboxing, enabling secret exfiltration by untrusted config
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/agents/utils.ts, line 137:
<comment>`file://` prompt_append reads arbitrary filesystem paths (including absolute/home) without sandboxing, enabling secret exfiltration by untrusted config</comment>
<file context>
@@ -125,15 +125,24 @@ function mergeAgentConfig(
+ }
+
+ if (path) {
+ const abs = path.startsWith("~/")
+ ? resolve(homedir(), path.slice(2))
+ : isAbsolute(path)
</file context>
Merge Conflict Resolution NeededThis PR has merge conflicts with the current Please rebase on latest git fetch origin
git checkout prompt-append-file-uri
git rebase origin/dev
git push --force-with-leaseSummary of this PR (still valid):
Note from Oracle review:
Once rebased, this PR is ready to merge. 🚀 |
devxoul
force-pushed
the
prompt-append-file-uri
branch
from
ea65b28 to
96e7b39
Compare
Properly handle encoded characters (e.g., %20 for spaces) when resolving file:// URIs in prompt_append configuration. Adds decodeURIComponent() to unwrap percent-encoded paths before filesystem access. Includes test case verifying percent-encoded paths with spaces can be loaded.
Add try-catch wrapper around decodeURIComponent() to gracefully handle invalid percent-encoding sequences (e.g., file://my%prompt.md). Shows warning message instead of crashing with URIError. Add test case verifying warning output for malformed URIs.
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/agents/utils.ts">
<violation number="1" location="src/agents/utils.ts:221">
P2: readFileSync is called after existsSync without any error handling, so directories or permission errors will throw and crash agent initialization.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| : isAbsolute(path) | ||
| ? path | ||
| : resolve(configDir ?? process.cwd(), path) | ||
| resolved = existsSync(abs) |
There was a problem hiding this comment.
P2: readFileSync is called after existsSync without any error handling, so directories or permission errors will throw and crash agent initialization.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/agents/utils.ts, line 221:
<comment>readFileSync is called after existsSync without any error handling, so directories or permission errors will throw and crash agent initialization.</comment>
<file context>
@@ -183,21 +187,43 @@ function applyOverrides(
+ : isAbsolute(path)
+ ? path
+ : resolve(configDir ?? process.cwd(), path)
+ resolved = existsSync(abs)
+ ? readFileSync(abs, "utf-8")
+ : `[WARNING: Could not resolve file URI: ${prompt_append}]`
</file context>
3 participants
Summary
Add
file://URI support inprompt_appendto load agent instructions from external files.Examples
Load from home directory:
{ "agents": { "oracle": { "prompt_append": "file://~/.config/opencode/my-oracle-rules.md" } } }Load project-specific context:
{ "agents": { "Sisyphus": { "prompt_append": "file://./docs/ai-context.md" } } }Combine with model override:
{ "agents": { "librarian": { "model": "anthropic/claude-sonnet-4", "prompt_append": "file://~/.config/opencode/research-guidelines.md" } } }Supported Formats
file:///Users/me/prompt.md/Users/me/prompt.mdfile://./context.md{project}/context.mdfile://~/prompts/rules.md~/prompts/rules.mdReturns
[WARNING: Could not resolve file URI: ...]if file not found.