Skip to content

feat(qoder): add Qoder rules support (1/4)#1600

Open
zxhdaniel wants to merge 1 commit into
dyoshikawa:mainfrom
zxhdaniel:feat/qoder-rules
Open

feat(qoder): add Qoder rules support (1/4)#1600
zxhdaniel wants to merge 1 commit into
dyoshikawa:mainfrom
zxhdaniel:feat/qoder-rules

Conversation

@zxhdaniel
Copy link
Copy Markdown

Summary

Add Qoder as a new tool target with rules support — the first PR in a series of 4 to add full Qoder integration.

About Qoder:
Qoder is an AI coding tool developed by Alibaba Group, designed for enterprise-grade intelligent code generation and development assistance. It supports IDE integration (JetBrains, VS Code) with features including rules, MCP, commands, subagents, skills, and hooks.

Changes

  • Add "qoder" to ALL_TOOL_TARGETS
  • Add qoder-specific fields (trigger, alwaysApply, description, globs) to RulesyncRuleFrontmatterSchema
  • Implement QoderRule adapter for .qoder/rules/*.md with YAML frontmatter
  • Smart trigger inference maps rulesync canonical fields (description, globs, cursor.alwaysApply) to Qoder's four native trigger modes: always_on, glob, model_decision, manual
  • Register QoderRule in rules-processor
  • Add .qoder/rules/ gitignore entry

PR Series

This is PR 1 of 4 for full Qoder support:

  1. Rules (this PR)
  2. MCP + Commands
  3. Subagents + Skills
  4. Hooks

Test plan

  • All existing tests pass (pnpm test)
  • Type checking passes (tsgo --noEmit)
  • Format check passes (oxfmt --check .)
  • Verified rulesync generate --targets "qoder" --features "rules" produces correct YAML frontmatter

Made with Cursor

Add Qoder (https://qoder.com) as a new tool target with rules support.

Qoder is an AI coding tool developed by Alibaba Group, designed for
enterprise-grade intelligent code generation and development assistance.

- Add "qoder" to ALL_TOOL_TARGETS
- Add qoder-specific fields to RulesyncRuleFrontmatterSchema
- Implement QoderRule adapter for .qoder/rules/*.md with YAML frontmatter
- Smart trigger inference maps rulesync canonical fields to Qoder's four
  native trigger modes: always_on, glob, model_decision, manual
- Register QoderRule in rules-processor
- Add .qoder/rules/ gitignore entry

Qoder documentation: https://docs.qoder.com/user-guide/rules

Co-authored-by: Cursor <cursoragent@cursor.com>

AI-Contributed/Feature: 0/285
AI-Contributed/UT: 0/501
Copy link
Copy Markdown
Owner

@dyoshikawa dyoshikawa left a comment

Choose a reason for hiding this comment

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

Nice work landing the Qoder adapter — the structure mirrors CursorRule cleanly and the trigger-mode inference logic is easy to follow. A few things stood out though.

The project's CLAUDE.md and .claude/rules/feature-change-guidelines.md both require: keeping the Tool × Feature happy-path matrix in src/e2e/e2e-rules.spec.ts (a qoder entry should be added there), updating the supported tools tables in README.md and docs/reference/supported-tools.md, adding a Qoder section to docs/reference/file-formats.md, and regenerating .gitignore via pnpm dev gitignore so the new **/.qoder/rules/ entry is committed. None of these landed in this PR — could you fold them in before merge?

A couple of inference-logic and test-rigor points are inline below. No security concerns: paths are constructed via well-vetted helpers, the schema uses safeParse consistently, and there's no shell/network/template surface.

Severity legend used inline: HIGH = blocks merge per project guidelines, MID = correctness/test-rigor, LOW = nice-to-have.

{ target: "pi", feature: "skills", entry: "**/.pi/skills/" },

// Qoder
{ target: "qoder", feature: "rules", entry: "**/.qoder/rules/" },
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

[MID] Registry entry looks right, but feature-change-guidelines.md also requires running pnpm dev gitignore and committing the regenerated /workspace/.gitignore. The repo's checked-in .gitignore doesn't yet include **/.qoder/rules/.

} from "./tool-rule.js";

export const QoderRuleFrontmatterSchema = z.looseObject({
trigger: z.optional(z.string()),
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

[LOW] trigger is typed as a free-form string, but per the PR description Qoder accepts exactly four modes (always_on / glob / model_decision / manual). A typo like "alway_on" would silently pass validation and produce a broken rule file. Consider tightening to z.union([z.literal("always_on"), z.literal("glob"), z.literal("model_decision"), z.literal("manual")]) here and on the matching qoder.trigger field in rulesync-rule.ts. (looseObject for the surrounding shape is still fine — this is just about the value of trigger itself.)

/**
* Infer the best Qoder trigger mode from rulesync canonical frontmatter.
*
* Priority: explicit qoder section > cursor hints > common fields.
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

[LOW] The JSDoc says "explicit qoder section > cursor hints > common fields", but lines 156–157 only fall back to fm.cursor?.alwaysApplyglobs and description skip the cursor section entirely. Either thread fm.cursor?.globs / fm.cursor?.description into the chain, or update the comment to match the actual behavior ("cursor.alwaysApply only").

trigger: fm.qoder.trigger,
...(fm.qoder.alwaysApply !== undefined && { alwaysApply: fm.qoder.alwaysApply }),
...(fm.qoder.description !== undefined && { description: fm.qoder.description }),
...(fm.qoder.globs !== undefined && { glob: fm.qoder.globs.join(",") }),
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

[MID] globs.join(",") produces e.g. "*.ts,*.js" (no space), and the reader at lines 84–86 trims either form. There's no test asserting the exact emitted string, so if Qoder's parser actually requires "*.ts, *.js" with a space (worth checking against docs.qoder.com/user-guide/rules), it would silently produce broken output. Could you add a test that locks down the literal emitted format and confirm against the Qoder spec? Same concern at line 168.


// Infer from common / cursor fields
const alwaysApply = fm.qoder?.alwaysApply ?? fm.cursor?.alwaysApply;
const globs = fm.qoder?.globs ?? fm.globs;
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

[LOW] See the JSDoc comment above — globs and description here ignore fm.cursor?.globs / fm.cursor?.description, so a rulesync file authored only with cursor: { globs: ["*.ts"] } and no top-level globs falls through to manual rather than glob. Probably not what users would expect.

expect(qoderRule.getFrontmatter().description).toBe("Qoder specific desc");
});

it("should infer always_on from cursor.alwaysApply", () => {
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

[MID] This test sets both globs: ["**/*"] AND cursor: { alwaysApply: true }, so the isCatchAll branch already evaluates to true at line 165 of the source — the assertion would still pass even if the ?? fm.cursor?.alwaysApply fallback at line 155 were removed. To actually exercise the cursor fallback path, drop globs: ["**/*"] (or set it to []) so only cursor.alwaysApply can drive the result.

});

describe("toRulesyncRule", () => {
it("should convert QoderRule to RulesyncRule preserving frontmatter", () => {
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

[LOW] Test description claims "preserving frontmatter" but only spot-checks three fields. Worth also asserting qoder.description (which toRulesyncRule does set) and the resulting globs (which becomes ["**/*"] because alwaysApply: true) so the round-trip contract is locked down.

});

describe("forDeletion", () => {
it("should create a QoderRule instance for deletion", () => {
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

[LOW] The deletion test only asserts instanceof QoderRule. A regression that swapped relativeDirPath / relativeFilePath would slip through. Mirror the cursor deletion tests by asserting getRelativeDirPath(), getRelativeFilePath(), and isDeletable().

globs: z.optional(z.array(z.string())),
}),
),
qoder: z.optional(
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

[LOW] Worth a one-line comment here noting the asymmetry: canonical rulesync stores qoder.globs: string[], but the emitted Qoder file uses glob: string (comma-separated). Easy to miss when extending.

@dyoshikawa
Copy link
Copy Markdown
Owner

@zxhdaniel Please work on the ci failure.

1 similar comment
@dyoshikawa
Copy link
Copy Markdown
Owner

@zxhdaniel Please work on the ci failure.

Copy link
Copy Markdown
Collaborator

@dyoshikawa-claw dyoshikawa-claw left a comment

Choose a reason for hiding this comment

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

The implementation looks solid and follows the existing tool rule patterns closely. The trigger inference logic is well thought out, and the test coverage is thorough.

A few things I'd recommend addressing before merging:

  • The !=!== issue in fromRulesyncRule is breaking CI (oxlint flags abstract equality). This needs fixing first.
  • E2E test entries for Qoder are missing in src/e2e/e2e-rules.spec.ts — both the generate and import test tables should have a "qoder" row to maintain Tool × Feature matrix coverage.
  • The supported tools table in README.md and docs/reference/supported-tools.md needs a Qoder row.

const description = fm.qoder?.description ?? fm.description;

const isCatchAll =
globs != null && globs.length > 0 && globs.every((g) => g === "**/*" || g === "**");
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

!= should be !== here — oxlint flags abstract equality. There are two occurrences of globs != null in the inference block (this line and line 161) that are breaking CI. Changing both to globs !== null will fix the failing Code Quality & Tests check.

} from "./tool-rule.js";

export const QoderRuleFrontmatterSchema = z.looseObject({
trigger: z.optional(z.string()),
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Consider tightening trigger from z.string() to something like z.union([z.literal("always_on"), z.literal("glob"), z.literal("manual"), z.literal("model_decision"), z.string()]). AntigravityRule uses this pattern — it documents valid values while keeping forward-compatibility via the trailing z.string(). Not blocking, just a suggestion.

trigger: fm.qoder.trigger,
...(fm.qoder.alwaysApply !== undefined && { alwaysApply: fm.qoder.alwaysApply }),
...(fm.qoder.description !== undefined && { description: fm.qoder.description }),
...(fm.qoder.globs !== undefined && { glob: fm.qoder.globs.join(",") }),
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

When qoder.globs is an empty array [], this still produces glob: "" in the YAML output since the check is !== undefined (empty arrays are truthy). Adding a length guard (fm.qoder.globs.length > 0) would avoid generating an empty glob value.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants