Skip to content

fix: don't fallback to relative path if not markdown file #9266

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

Merged
sestinj merged 3 commits into from
Dec 22, 2025
Merged

fix: don't fallback to relative path if not markdown file #9266

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d93b432
fix: don't fallback to relative path if not markdown file
RomneyDa Dec 22, 2025
4c0c532
fix: fix lint error and update tests for markdown-only fallback
sestinj Dec 22, 2025
53a9fa4
fix: add missing hub mock rejection in test
sestinj Dec 22, 2025
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
66 changes: 30 additions & 36 deletions extensions/cli/src/services/AgentFileService.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -525,17 +525,17 @@ You are a helpful agent`;
expect(result).toEqual(mockAgentFile);
});

it("should fallback to file path when hub loading fails for slug-like path", async () => {
it("should fallback to file path when hub loading fails for slug-like path with .md extension", async () => {
mockLoadPackageFromHub.mockRejectedValue(new Error("Hub error"));
mockPathResolve.mockImplementation((p: string) => `/resolved/${p}`);
mockReadFileSync.mockReturnValue(mockFileContent);

const result = await agentFileService.getAgentFile("owner/agent");
const result = await agentFileService.getAgentFile("owner/agent.md");

expect(mockLoadPackageFromHub).toHaveBeenCalled();
expect(mockPathResolve).toHaveBeenCalledWith("owner/agent");
expect(mockLoadPackageFromHub).toHaveBeenCalled(); // Two-part paths try hub first
expect(mockPathResolve).toHaveBeenCalledWith("owner/agent.md");
expect(mockReadFileSync).toHaveBeenCalledWith(
"/resolved/owner/agent",
"/resolved/owner/agent.md",
"utf-8",
);
expect(result).toBeDefined();
Expand Down Expand Up @@ -641,13 +641,9 @@ You are a helpful agent`;

describe("path format edge cases", () => {
it("should treat single-part path as file path, not hub slug", async () => {
const singlePath = "agent";
mockPathResolve.mockReturnValue("/resolved/agent");
const singlePath = "agent.md";
mockPathResolve.mockReturnValue("/resolved/agent.md");
mockReadFileSync.mockReturnValue(mockFileContent);
// Make hub loading fail so it falls back to file system
mockLoadPackageFromHub.mockRejectedValueOnce(
new Error("Not a hub slug"),
);

const result = await agentFileService.getAgentFile(singlePath);

Expand All @@ -658,13 +654,9 @@ You are a helpful agent`;
});

it("should treat three-part path as file path, not hub slug", async () => {
const threePath = "path/to/agent";
mockPathResolve.mockReturnValue("/resolved/path/to/agent");
const threePath = "path/to/agent.md";
mockPathResolve.mockReturnValue("/resolved/path/to/agent.md");
mockReadFileSync.mockReturnValue(mockFileContent);
// Make hub loading fail so it falls back to file system
mockLoadPackageFromHub.mockRejectedValueOnce(
new Error("Not a hub slug"),
);

const result = await agentFileService.getAgentFile(threePath);

Expand All @@ -675,29 +667,20 @@ You are a helpful agent`;
});

it("should not treat two-part path with empty part as hub slug", async () => {
const emptyPartPath = "owner/";
mockPathResolve.mockReturnValue("/resolved/owner/");
mockReadFileSync.mockReturnValue(mockFileContent);
// Make hub loading fail so it falls back to file system
mockLoadPackageFromHub.mockRejectedValueOnce(
new Error("Not a hub slug"),
);
const emptyPartPath = "owner/"; // Empty second part

const result = await agentFileService.getAgentFile(emptyPartPath);
// With the new behavior, non-markdown paths throw errors
await expect(
agentFileService.getAgentFile(emptyPartPath),
).rejects.toThrow("Not a markdown file");

expect(mockLoadPackageFromHub).not.toHaveBeenCalled();
expect(mockPathResolve).toHaveBeenCalledWith(emptyPartPath);
expect(result).toBeDefined();
expect(mockLoadPackageFromHub).not.toHaveBeenCalled(); // Empty part means no hub attempt
});

it("should not treat path starting with slash as hub slug", async () => {
const slashPath = "/owner/agent";
const slashPath = "/owner/agent.md";
mockPathResolve.mockReturnValue(slashPath);
mockReadFileSync.mockReturnValue(mockFileContent);
// Make hub loading fail so it falls back to file system
mockLoadPackageFromHub.mockRejectedValueOnce(
new Error("Not a hub slug"),
);

const result = await agentFileService.getAgentFile(slashPath);

Expand Down Expand Up @@ -741,19 +724,30 @@ You are a helpful agent`;
).rejects.toThrow("Failed to load agent from ./invalid.md");
});

it("should throw error when hub loading fails and file fallback also fails", async () => {
it("should throw error when file reading fails for markdown path", async () => {
Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot Dec 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

P1: Test is missing hub mock rejection. For two-part paths like owner/agent.md, the implementation tries hub first (per test at line 529). Without mockLoadPackageFromHub.mockRejectedValue(), hub loading succeeds and returns mockAgentFile, never reaching the file reading code being tested.

Prompt for AI agents 
Check if this issue is valid — if so, understand the root cause and fix it. At extensions/cli/src/services/AgentFileService.test.ts, line 727:

<comment>Test is missing hub mock rejection. For two-part paths like `owner/agent.md`, the implementation tries hub first (per test at line 529). Without `mockLoadPackageFromHub.mockRejectedValue()`, hub loading succeeds and returns `mockAgentFile`, never reaching the file reading code being tested.</comment>

<file context>
@@ -741,19 +724,29 @@ You are a helpful agent`;
-      it(&quot;should throw error when hub loading fails and file fallback also fails&quot;, async () =&gt; {
-        mockLoadPackageFromHub.mockRejectedValue(new Error(&quot;Hub error&quot;));
-        mockPathResolve.mockReturnValue(&quot;/resolved/owner/agent&quot;);
+      it(&quot;should throw error when file reading fails for markdown path&quot;, async () =&gt; {
+        mockPathResolve.mockReturnValue(&quot;/resolved/owner/agent.md&quot;);
         mockReadFileSync.mockImplementation(() =&gt; {
</file context>

✅ Addressed in 53a9fa4

mockLoadPackageFromHub.mockRejectedValue(new Error("Hub error"));
mockPathResolve.mockReturnValue("/resolved/owner/agent");
mockPathResolve.mockReturnValue("/resolved/owner/agent.md");
mockReadFileSync.mockImplementation(() => {
throw new Error("File not found");
});

await expect(
agentFileService.getAgentFile("owner/agent.md"),
).rejects.toThrow("Failed to load agent from owner/agent.md");
await expect(
agentFileService.getAgentFile("owner/agent.md"),
).rejects.toThrow("File not found");
});

it("should throw error for non-markdown paths without trying file fallback", async () => {
mockLoadPackageFromHub.mockRejectedValue(new Error("Hub error"));

await expect(
agentFileService.getAgentFile("owner/agent"),
).rejects.toThrow("Failed to load agent from owner/agent");
await expect(
agentFileService.getAgentFile("owner/agent"),
).rejects.toThrow("File not found");
).rejects.toThrow("Not a markdown file");
});

it("should handle permission errors when reading files", async () => {
Expand Down
15 changes: 11 additions & 4 deletions extensions/cli/src/services/AgentFileService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,13 +58,20 @@ export class AgentFileService
if (parts.length === 2 && parts[0] && parts[1] && !parts.includes(".")) {
try {
return await loadPackageFromHub(agentPath, agentFileProcessor);
} catch (e) {
logger.info(
`Failed to load agent file from slug-like path ${agentPath}: ${getErrorString(e)}`,
);
} catch {
// slug COULD be path, fall back to relative path
}
}

const isMarkdownPath =
agentPath.endsWith(".md") || agentPath.endsWith(".markdown");
// Only fall back to relative path for markdown files
if (!isMarkdownPath) {
throw new Error(
`Failed to load agent from ${agentPath}. Not a markdown file`,
);
}

const resolvedPath = agentPath.startsWith("file:/")
? fileURLToPath(agentPath)
: path.resolve(agentPath);
Expand Down
Loading