Skip to content

test: patch — maybeParseApplyPatchVerified coverage #396

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.

Sign up for GitHub

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

Closed
anandgupta42 wants to merge 1 commit into from
Closed

test: patch — maybeParseApplyPatchVerified coverage #396

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
161 changes: 161 additions & 0 deletions packages/opencode/test/patch/patch.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,6 +261,167 @@ PATCH`
})
})

describe("maybeParseApplyPatchVerified", () => {
test("detects implicit invocation (raw patch without apply_patch command)", async () => {
const patchText = `*** Begin Patch
*** Add File: test.txt
+Content
*** End Patch`

const result = await Patch.maybeParseApplyPatchVerified([patchText], tempDir)
expect(result.type).toBe(Patch.MaybeApplyPatchVerified.CorrectnessError)
if (result.type === Patch.MaybeApplyPatchVerified.CorrectnessError) {
expect(result.error.message).toBe(Patch.ApplyPatchError.ImplicitInvocation)
}
})

test("returns NotApplyPatch for single arg that is not a valid patch", async () => {
const result = await Patch.maybeParseApplyPatchVerified(["echo hello"], tempDir)
expect(result.type).toBe(Patch.MaybeApplyPatchVerified.NotApplyPatch)
})

test("returns NotApplyPatch for unrelated multi-arg commands", async () => {
const result = await Patch.maybeParseApplyPatchVerified(["ls", "-la", "/tmp"], tempDir)
expect(result.type).toBe(Patch.MaybeApplyPatchVerified.NotApplyPatch)
})

test("returns Body with add change for apply_patch add hunk", async () => {
const patchText = `*** Begin Patch
*** Add File: new-file.txt
+line one
+line two
*** End Patch`

const result = await Patch.maybeParseApplyPatchVerified(["apply_patch", patchText], tempDir)
expect(result.type).toBe(Patch.MaybeApplyPatchVerified.Body)
if (result.type === Patch.MaybeApplyPatchVerified.Body) {
const resolvedPath = path.resolve(tempDir, "new-file.txt")
const change = result.action.changes.get(resolvedPath)
expect(change).toBeDefined()
expect(change!.type).toBe("add")
if (change!.type === "add") {
expect(change!.content).toBe("line one\nline two")
}
expect(result.action.cwd).toBe(tempDir)
}
})

test("returns Body with delete change including file content", async () => {
const filePath = path.join(tempDir, "to-delete.txt")
await fs.writeFile(filePath, "original content here")

const patchText = `*** Begin Patch
*** Delete File: to-delete.txt
*** End Patch`

const result = await Patch.maybeParseApplyPatchVerified(["apply_patch", patchText], tempDir)
expect(result.type).toBe(Patch.MaybeApplyPatchVerified.Body)
if (result.type === Patch.MaybeApplyPatchVerified.Body) {
const resolvedPath = path.resolve(tempDir, "to-delete.txt")
const change = result.action.changes.get(resolvedPath)
expect(change).toBeDefined()
expect(change!.type).toBe("delete")
if (change!.type === "delete") {
expect(change!.content).toBe("original content here")
}
}
})

test("returns CorrectnessError when deleting non-existent file", async () => {
const patchText = `*** Begin Patch
*** Delete File: no-such-file.txt
*** End Patch`

const result = await Patch.maybeParseApplyPatchVerified(["apply_patch", patchText], tempDir)
expect(result.type).toBe(Patch.MaybeApplyPatchVerified.CorrectnessError)
if (result.type === Patch.MaybeApplyPatchVerified.CorrectnessError) {
expect(result.error.message).toContain("Failed to read file for deletion")
}
})

test("returns Body with update change including unified_diff and new_content", async () => {
const filePath = path.join(tempDir, "to-update.txt")
await fs.writeFile(filePath, "alpha\nbeta\ngamma\n")

const patchText = `*** Begin Patch
*** Update File: to-update.txt
@@
alpha
-beta
+BETA
gamma
*** End Patch`

const result = await Patch.maybeParseApplyPatchVerified(["apply_patch", patchText], tempDir)
expect(result.type).toBe(Patch.MaybeApplyPatchVerified.Body)
if (result.type === Patch.MaybeApplyPatchVerified.Body) {
const resolvedPath = path.resolve(tempDir, "to-update.txt")
const change = result.action.changes.get(resolvedPath)
expect(change).toBeDefined()
expect(change!.type).toBe("update")
if (change!.type === "update") {
expect(change!.new_content).toBe("alpha\nBETA\ngamma\n")
expect(change!.unified_diff).toContain("-beta")
expect(change!.unified_diff).toContain("+BETA")
expect(change!.move_path).toBeUndefined()
}
}
})

test("returns CorrectnessError when updating file with non-matching old_lines", async () => {
const filePath = path.join(tempDir, "mismatch.txt")
await fs.writeFile(filePath, "actual content\n")

const patchText = `*** Begin Patch
*** Update File: mismatch.txt
@@
-completely different content
+replacement
*** End Patch`

const result = await Patch.maybeParseApplyPatchVerified(["apply_patch", patchText], tempDir)
expect(result.type).toBe(Patch.MaybeApplyPatchVerified.CorrectnessError)
if (result.type === Patch.MaybeApplyPatchVerified.CorrectnessError) {
expect(result.error.message).toContain("Failed to find expected lines")
}
})

test("resolves move_path for update with move directive", async () => {
const filePath = path.join(tempDir, "old-name.txt")
await fs.writeFile(filePath, "keep this\n")

const patchText = `*** Begin Patch
*** Update File: old-name.txt
*** Move to: new-name.txt
@@
-keep this
+keep that
*** End Patch`

const result = await Patch.maybeParseApplyPatchVerified(["apply_patch", patchText], tempDir)
expect(result.type).toBe(Patch.MaybeApplyPatchVerified.Body)
if (result.type === Patch.MaybeApplyPatchVerified.Body) {
// The change is keyed by the resolved move_path, not the original path
const resolvedMovePath = path.resolve(tempDir, "new-name.txt")
const change = result.action.changes.get(resolvedMovePath)
expect(change).toBeDefined()
expect(change!.type).toBe("update")
if (change!.type === "update") {
expect(change!.move_path).toBe(resolvedMovePath)
expect(change!.new_content).toBe("keep that\n")
}
}
})

test("returns CorrectnessError for invalid patch syntax", async () => {
const result = await Patch.maybeParseApplyPatchVerified(
["apply_patch", "this is not a valid patch at all"],
tempDir,
)
expect(result.type).toBe(Patch.MaybeApplyPatchVerified.CorrectnessError)
})
})
Comment on lines +264 to +423
Copy link

@coderabbitai coderabbitai bot Mar 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Use the repo tmp fixture pattern in these new tests.

The new tests keep depending on a manually-created shared temp directory. In this test path, the project standard is await using tmp = await tmpdir(...) and reading the realpath-resolved directory from tmp.path. Please migrate these new cases to that pattern.

Suggested pattern for each new test 
test("...", async () => {
  await using tmp = await tmpdir()
  const tempDir = tmp.path

  const result = await Patch.maybeParseApplyPatchVerified([...], tempDir)
  // assertions...
})

As per coding guidelines: “Use the tmpdir function from fixture/fixture.ts… Always use await using syntax… Access the temporary directory path via the path property and ensure paths are realpath resolved”.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@packages/opencode/test/patch/patch.test.ts` around lines 264 - 423, The tests
in this block use a manually-created shared tempDir instead of the repo fixture
pattern; update each test to obtain an isolated temp directory via the tmpdir
fixture using "await using tmp = await tmpdir()" and pass tmp.path (realpath
resolved) into Patch.maybeParseApplyPatchVerified instead of the existing
tempDir variable; ensure any files are created under that tmp.path and any
expected resolved paths use path.resolve(tmp.path, ...). Target symbols:
Patch.maybeParseApplyPatchVerified, tmpdir fixture (from fixture/fixture.ts),
and any local tempDir usages in these tests.


describe("error handling", () => {
test("should throw error when updating non-existent file", async () => {
const nonExistent = path.join(tempDir, "does-not-exist.txt")
Expand Down
Loading