Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
25 changes: 23 additions & 2 deletions skills/coderuler-new-rule/reference.md
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ Default-export a constructible class extending `Rule<T>` (`T = JsonValue`). Extr
| `scope?: ScopeSelector<T>` | Absent | One whole-file scope per eligible file unless supplied. |
| `settings: RuleSettings` | `{}` | Optional backend, model, threshold, and input/output USD-per-million token prices; see AI below. |
| `group: Grouping` | `'scope'` | Groups surviving scopes before evaluation. |
| `comment?: string` | Absent | Nonblank Markdown override for each GitHub finding body with `--auto-comment`; see below. |
| `comment` (optional) | Absent | Nonblank Markdown string or callback for each GitHub finding; see below. |
| `when(context: ProjectContext): Awaitable<PredicateResult>` | `true` | Whole-rule applicability, once per enabled rule before parsing target sources. |
| `select(context: SelectionContext): Awaitable<readonly ScopeSubject<T>[]>` | Delegates | Calls `context.select({ files, exclude, scope })`; override for custom selection. |
| `filter(subject: ScopeSubject<T>, context: ProjectContext): Awaitable<PredicateResult>` | `true` | Per-scope applicability before grouping. |
Expand Down Expand Up @@ -317,7 +317,28 @@ Explicit `lines` are absolute file coordinates: positive safe integers with incl

With `locations` omitted, a scope violation uses its own source lines; a group violation does **not** automatically blame all members. Explicit `locations: []` disables automatic scope attribution. Without usable source lines (or a matching current PR diff anchor), `--auto-comment` reports the finding in an ordinary PR comment instead of guessing an inline location.

Set `comment` on the rule to replace each generated GitHub finding body with your Markdown. Omit it to keep the generated body. CodeRuler strips terminal controls and redacts credentials from overrides. Inline locations and check results stay unchanged. The fallback comment keeps its shared `CodeRuler violations` heading.
Set `comment` to a nonblank Markdown string to use the same body for each finding. You can also set a callback to build the body from the evaluated subject and its normalized `CheckResult`.

The callback returns a string or a promise for a string. CodeRuler calls it once for each violating evaluation, after the check finishes. A grouped finding with many locations still gets one callback call. CodeRuler skips callbacks for passing evaluations and runs them only with `--auto-comment`.

```ts
import { Rule, violation, type CheckResult, type Subject } from '@alex-dbs/coderuler';

type PolicyData = { name: string };

class NamePolicy extends Rule<PolicyData> {
check = () => violation('Policy violation');

comment = function (this: Rule<PolicyData>, subject: Subject<PolicyData>, result: CheckResult) {
const target = subject.kind === 'scope' ? subject.filepath : subject.group.path;
return `**${this.group} ${target}**: ${result.message ?? 'Policy violation'}`;
};
}
```

A normal function receives the rule instance as `this`, typed as `Rule<T>`. An arrow function keeps its lexical `this`. Use it to access subclass-only fields.

Return a nonblank string. A throw, rejected promise, blank string, or non-string result fails comment rendering. The CLI exits with status `2`. CodeRuler does not publish comments or use a generated body for that run. CodeRuler strips terminal controls and redacts credentials from resolved bodies. Markdown stays unescaped. The callback does not change check results or inline locations. Reports and JSON do not include computed bodies.

Scalar AI classification supplies no offending-member attribution: scope AI can use its subject's lines, but grouped AI needs explicit locations from a custom check wrapping `context.ai(...)`; otherwise it uses the fallback comment.

Expand Down
2 changes: 1 addition & 1 deletion src/adapter/cli/commands/check.ts
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ async function runCheck(
if (options.autoComment === true) {
await services.commentOnPullRequest(
root,
renderViolationComments(reports, context.redact, rules),
await renderViolationComments(reports, context.redact, rules),
checkout?.pullRequest,
);
}
Expand Down
28 changes: 21 additions & 7 deletions src/adapter/cli/reports.ts
Original file line number Diff line number Diff line change
Expand Up @@ -671,16 +671,24 @@ export function countViolations(reports: readonly RuleReport[]): number {
return total;
}

export function renderViolationComments(
export async function renderViolationComments(
reports: readonly RuleReport[],
redact: TextRedactor,
rules?: readonly LoadedRule[],
): readonly ViolationComment[] {
const overrides = new Map<string, string>();
): Promise<readonly ViolationComment[]> {
const overrides = new Map<string, {
readonly rule: LoadedRule['rule'];
readonly comment: NonNullable<LoadedRule['rule']['comment']>;
}>();
if (rules !== undefined) {
for (const entry of rules) {
const comment = entry.rule.comment;
if (comment !== undefined) overrides.set(entry.name, redact(stripVTControlCharacters(comment)));
if (comment !== undefined) {
overrides.set(entry.name, {
rule: entry.rule,
comment: typeof comment === 'string' ? redact(stripVTControlCharacters(comment)) : comment,
});
}
}
}
const safe = (text: string): string => escapeText(redact(stripVTControlCharacters(text)))
Expand All @@ -705,9 +713,7 @@ export function renderViolationComments(
for (const { subject, result } of report.evaluations) {
if (result.status !== 'violation') continue;
let body: string;
if (override !== undefined) {
body = override;
} else {
if (override === undefined) {
const label = subject.kind === 'scope'
? `scope ${location(subject)} (${safe(subject.id)})`
: safe(`group ${subject.group.type} ${subject.group.path} (${subject.members.length} scopes)`);
Expand All @@ -720,6 +726,14 @@ export function renderViolationComments(
}
if (result.message !== undefined) lines.push(` - ${safe(result.message)}`);
body = `${lines.join('\n')}\n`;
} else if (typeof override.comment === 'string') {
body = override.comment;
} else {
const value: unknown = await override.comment.call(override.rule, subject, result);
if (typeof value !== 'string' || value.trim() === '') {
throw new Error(`Rule "${report.name}" comment result must be a non-blank string`);
}
body = redact(stripVTControlCharacters(value));
}
const candidates = result.locations
?? (subject.kind === 'scope' ? [subject] : []);
Expand Down
2 changes: 1 addition & 1 deletion src/app/rule.ts
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,7 @@ export abstract class Rule<T = JsonValue> {
scope?: ScopeSelector<T>;
settings: RuleSettings = {};
group: Grouping = 'scope';
comment?: string;
comment?: string | ((this: Rule<T>, subject: Subject<T>, result: CheckResult) => Awaitable<string>);

when(_context: ProjectContext): Awaitable<PredicateResult> {
return true;
Expand Down
4 changes: 2 additions & 2 deletions src/app/validate.ts
Original file line number Diff line number Diff line change
Expand Up @@ -98,8 +98,8 @@ export function validateRule(value: unknown, ruleFile: string): Rule<unknown> {
validateSettings(settings, at);

const comment = value['comment'];
if (comment !== undefined && (typeof comment !== 'string' || comment.trim() === '')) {
invalid(at, 'comment must be a non-blank string');
if (comment !== undefined && typeof comment !== 'function' && (typeof comment !== 'string' || comment.trim() === '')) {
invalid(at, 'comment must be a non-blank string or a function');
}

if (!isGrouping(value['group'])) {
Expand Down
150 changes: 148 additions & 2 deletions tests/cli-auto-comment.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,14 +29,15 @@ interface Fixture extends Sandbox {
readonly head: string;
}

const scoped = (check: string, options = '') => `import { Rule, pass, violation, regex } from '@alex-dbs/coderuler';
const scoped = (check: string, options = '', privateData = 'DATA-SECRET') => `import { Rule, pass, violation, regex } from '@alex-dbs/coderuler';
export default class FixtureRule extends Rule {
files = ['src/*.txt'];
scope = file => [{ id: '$', source: file.source, data: { private: 'DATA-SECRET' }, captures: { private: 'CAPTURE-SECRET' }, lines: { start: 2, end: 3 } }];
scope = file => [{ id: '$', source: file.source, data: { private: ${JSON.stringify(privateData)} }, captures: { private: 'CAPTURE-SECRET' }, lines: { start: 2, end: 3 } }];
${options}
check = ${check};
}
`;

const implicit = scoped(`subject => subject.source.includes('BAD') ? violation(${JSON.stringify(MESSAGE)}) : pass('PASS-MESSAGE')`);
const unlocatedGroup = scoped(`() => violation('unlocated group rejection')`, `group = 'folder';`);

Expand Down Expand Up @@ -196,6 +197,53 @@ for (const diff of ['--diff', '--diff-only']) {
}

const CUSTOM = '**Policy** — follow [the guide](https://example.invalid/guide) and keep `snake_case`.\n\n- mention @scope';
for (const kind of ['sync', 'async'] as const) {
test(`subject-based comments (${kind})`, async context => {
const callback = `${kind === 'async' ? 'async ' : ''}function (subject, result) {
if (result.status !== 'violation') throw new Error('comment called for a passing result');
${kind === 'async' ? 'await Promise.resolve();' : ''}
const token = ${JSON.stringify(TOKEN)};
const apiKey = ${JSON.stringify(API_KEY)};
return this.guidance + ' ' + subject.filepath + ': ' + subject.data.private + '. ' + result.message + ' '
+ token.slice(0, 5) + ${JSON.stringify('\u001b[31m')} + token.slice(5) + ${JSON.stringify('\u001b[0m')} + ' '
+ apiKey.slice(0, 5) + ${JSON.stringify('\u001b[31m')} + apiKey.slice(5) + ${JSON.stringify('\u001b[0m')};
}`;
const target = repository(`subject-comment-${kind}`, {
custom: scoped(
`subject => subject.source.includes('BAD') ? violation('Needs trusted data') : pass()`,
`guidance = '**Policy**'; comment = ${callback};`,
'field-specific guidance',
),
});
const requests = await github(context, target);
const result = await check(target, ['--diff', '--base', target.base, '--auto-comment', '--json', '--output-all']);
assert.equal(result.status, 1, result.stderr);
assert.equal(result.stderr, '');
assert.equal(posts(requests).length, 1);
const payload = review(requests);
assert.deepEqual(payload.comments.map(({ path, body, start_line, line, side, start_side }) => ({
path, body, start_line, line, side, start_side,
})).sort((left, right) => left.path.localeCompare(right.path)), [
{
path: 'src/one.txt',
body: '**Policy** src/one.txt: field-specific guidance. Needs trusted data [REDACTED] [REDACTED]',
start_line: 2, line: 3, side: 'RIGHT', start_side: 'RIGHT',
},
{
path: 'src/two.txt',
body: '**Policy** src/two.txt: field-specific guidance. Needs trusted data [REDACTED] [REDACTED]',
start_line: 2, line: 3, side: 'RIGHT', start_side: 'RIGHT',
},
]);
const report = JSON.parse(result.stdout).reports[0];
const failures = report.evaluations.filter(({ result: finding }: { result: { status: string } }) => finding.status === 'violation');
assert.deepEqual(failures.map(({ result: finding }: { result: { message: string } }) => finding.message), [
'Needs trusted data', 'Needs trusted data',
]);
assert.ok(failures.every(({ result: finding }: { result: Record<string, unknown> }) => !Object.hasOwn(finding, 'comment')));
assert.equal(Object.hasOwn(report, 'comment'), false);
});
}

test('per-rule comments replace review bodies verbatim and preserve locations', async context => {
const target = repository('custom-review', {
Expand Down Expand Up @@ -247,6 +295,104 @@ test('a commented group rule uses the comment as the whole fallback finding', as
assert.equal(fallback(requests), `## CodeRuler violations\n\n${CUSTOM}\n`);
});

test('an async group callback uses group metadata for the fallback finding', async context => {
const target = repository('custom-async-fallback', {
grouped: scoped(`() => violation('group rejection')`, `group = 'folder'; comment = async function (subject, result) {
if (result.status !== 'violation' || subject.kind !== 'group') throw new Error('comment called for a non-group violation');
await Promise.resolve();
return '**' + this.group + ' ' + subject.group.path + '**: ' + result.message;
};`),
});
const requests = await github(context, target);
const result = await check(target, ['--diff', '--base', target.base, '--auto-comment']);
assert.equal(result.status, 1, result.stderr);
assert.equal(result.stderr, '');
assert.equal(posts(requests).length, 1);
assert.equal(fallback(requests), '## CodeRuler violations\n\n**folder src**: group rejection\n');
});

for (const kind of ['sync-throw', 'async-rejection'] as const) {
test(`${kind} comment failure stops publishing after an earlier result`, async context => {
const diagnostic = kind === 'sync-throw' ? TOKEN : API_KEY;
const failure = kind === 'sync-throw'
? `throw new Error(${JSON.stringify(diagnostic)});`
: `return Promise.reject(new Error(${JSON.stringify(diagnostic)}));`;
const callback = `${kind === 'async-rejection' ? 'async ' : ''}function (subject, result) {
if (result.status !== 'violation') throw new Error('comment called for a passing result');
if (subject.filepath === 'src/two.txt') {
if (!this.commentResolved) throw new Error('src/one.txt comment did not resolve first');
${failure}
}
this.commentResolved = true;
return 'resolved before the failure';
}`;
const target = repository(`comment-${kind}`, {
custom: scoped(`subject => subject.source.includes('BAD') ? violation('Needs trusted data') : pass()`, `comment = ${callback};`),
});
const requests = await github(context, target);
const result = await check(target, ['--diff', '--base', target.base, '--auto-comment', '--json', '--output-all']);
assert.equal(result.status, 2);
assert.match(result.stderr, /\[REDACTED\]/);
assert.equal(result.stderr.includes(TOKEN), false);
assert.equal(result.stderr.includes(API_KEY), false);
assert.ok(JSON.parse(result.stdout).reports[0].evaluations.some(({ result: finding }: { result: { message?: string } }) => finding.message === 'Needs trusted data'));
assert.deepEqual(posts(requests), []);
});
}

for (const [kind, invalid] of [
['number', '42'],
['undefined', 'undefined'],
['empty', "''"],
['whitespace', "' \\t\\n '"],
] as const) {
test(`async comment rejects ${kind} results after an earlier result`, async context => {
const callback = `async function (subject, result) {
if (result.status !== 'violation') throw new Error('comment called for a passing result');
if (subject.filepath === 'src/two.txt') {
if (!this.commentResolved) throw new Error('src/one.txt comment did not resolve first');
return ${invalid};
}
this.commentResolved = true;
return 'resolved before the invalid result';
}`;
const target = repository(`comment-invalid-${kind}`, {
custom: scoped(`subject => subject.source.includes('BAD') ? violation('Needs trusted data') : pass()`, `comment = ${callback};`),
});
const requests = await github(context, target);
const result = await check(target, ['--diff', '--base', target.base, '--auto-comment', '--json', '--output-all']);
assert.equal(result.status, 2);
assert.match(result.stderr, /Rule "custom" comment result must be a non-blank string/);
assert.ok(JSON.parse(result.stdout).reports[0].evaluations.some(({ result: finding }: { result: { message?: string } }) => finding.message === 'Needs trusted data'));
assert.deepEqual(posts(requests), []);
});
}

test('a throwing callback is not invoked without --auto-comment', async context => {
const target = repository('comment-without-publish', {
custom: scoped(
`subject => subject.source.includes('BAD') ? violation('Needs trusted data') : pass()`,
`comment = function () { throw new Error(${JSON.stringify(`must not run ${TOKEN}`)}); };`,
),
});
const requests = await github(context, target);
const result = await check(target, ['--diff', '--base', target.base, '--json', '--output-all']);
assert.equal(result.status, 1, result.stderr);
assert.equal(result.stderr, '');
const reports = JSON.parse(result.stdout).reports;
assert.deepEqual(reports.map((report: { name: string; status: string }) => ({ name: report.name, status: report.status })), [
{ name: 'custom', status: 'violation' },
]);
const findings: { status?: string; message?: string; comment?: unknown }[] = reports[0].evaluations.map(
({ result: finding }: { result: { status?: string; message?: string; comment?: unknown } }) => finding,
);
assert.deepEqual(findings.filter(finding => finding.status === 'violation').map(finding => finding.message), [
'Needs trusted data', 'Needs trusted data',
]);
assert.ok(findings.every(finding => !Object.hasOwn(finding, 'comment')));
assert.deepEqual(posts(requests), []);
});

test('commented findings strip terminal controls before redaction and keep Markdown unescaped', async context => {
const token = 'supersecretgithubtoken';
const apiKey = 'supersecrettypesafekey';
Expand Down
Loading
Loading