refactor: deep code review improvements
- Add *.log to .gitignore, remove debug.log from tracking - Fix dev-mode FS scan to discover .ts adapter files (not just .js) - Deduplicate CONNECT_TIMEOUT: browser.ts now uses runtime.ts constant - Fix CSV output: escape newlines in field values per RFC 4180 - Add proper type interfaces for validate/verify (remove any types) - Remove unused hadOuterQuotes variable in snapshotFormatter - Derive CliOptions from CliCommand via Omit+Partial to reduce duplication - Expand dense one-liner action callbacks in main.ts for readability
This commit is contained in:
1 parent
2652fa40e5
commit
7555f14369
9 files changed
+56
-25
No files matched your search
@@ -3,3 +3,4 @@ dist/
|
||||
*.tsbuildinfo
|
||||
.opencli/
|
||||
.mcp.json
|
||||
*.log
|
||||
+2
-4
@@ -13,9 +13,7 @@ import { formatSnapshot } from './snapshotFormatter.js';
|
||||
import { PKG_VERSION } from './version.js';
|
||||
import { normalizeEvaluateSource } from './pipeline/template.js';
|
||||
import { generateInterceptorJs, generateReadInterceptedJs } from './interceptor.js';
|
||||
import { withTimeoutMs } from './runtime.js';
|
||||
|
||||
const CONNECT_TIMEOUT = parseInt(process.env.OPENCLI_BROWSER_CONNECT_TIMEOUT ?? '30', 10);
|
||||
import { withTimeoutMs, DEFAULT_BROWSER_CONNECT_TIMEOUT } from './runtime.js';
|
||||
const STDERR_BUFFER_LIMIT = 16 * 1024;
|
||||
const INITIAL_TABS_TIMEOUT_MS = 1500;
|
||||
const TAB_CLEANUP_TIMEOUT_MS = 2000;
|
||||
@@ -344,7 +342,7 @@ export class PlaywrightMCP {
|
||||
PlaywrightMCP._registerGlobalCleanup();
|
||||
PlaywrightMCP._activeInsts.add(this);
|
||||
this._state = 'connecting';
|
||||
const timeout = opts.timeout ?? CONNECT_TIMEOUT;
|
||||
const timeout = opts.timeout ?? DEFAULT_BROWSER_CONNECT_TIMEOUT;
|
||||
|
||||
return new Promise<Page>((resolve, reject) => {
|
||||
const isDebug = process.env.DEBUG?.includes('opencli:mcp');
|
||||
|
||||
+4
-1
@@ -101,7 +101,10 @@ async function discoverClisFromFs(dir: string): Promise<void> {
|
||||
const filePath = path.join(siteDir, file);
|
||||
if (file.endsWith('.yaml') || file.endsWith('.yml')) {
|
||||
registerYamlCli(filePath, site);
|
||||
} else if (file.endsWith('.js') && !file.endsWith('.d.js')) {
|
||||
} else if (
|
||||
(file.endsWith('.js') && !file.endsWith('.d.js')) ||
|
||||
(file.endsWith('.ts') && !file.endsWith('.d.ts') && !file.endsWith('.test.ts'))
|
||||
) {
|
||||
promises.push(
|
||||
import(`file://${filePath}`).catch((err: any) => {
|
||||
process.stderr.write(`Warning: failed to load module ${filePath}: ${err.message}\n`);
|
||||
|
||||
+10
-2
@@ -62,10 +62,18 @@ program.command('list').description('List all available CLI commands').option('-
|
||||
});
|
||||
|
||||
program.command('validate').description('Validate CLI definitions').argument('[target]', 'site or site/name')
|
||||
.action(async (target) => { const { validateClisWithTarget, renderValidationReport } = await import('./validate.js'); console.log(renderValidationReport(validateClisWithTarget([BUILTIN_CLIS, USER_CLIS], target))); });
|
||||
.action(async (target) => {
|
||||
const { validateClisWithTarget, renderValidationReport } = await import('./validate.js');
|
||||
console.log(renderValidationReport(validateClisWithTarget([BUILTIN_CLIS, USER_CLIS], target)));
|
||||
});
|
||||
|
||||
program.command('verify').description('Validate + smoke test').argument('[target]').option('--smoke', 'Run smoke tests', false)
|
||||
.action(async (target, opts) => { const { verifyClis, renderVerifyReport } = await import('./verify.js'); const r = await verifyClis({ builtinClis: BUILTIN_CLIS, userClis: USER_CLIS, target, smoke: opts.smoke }); console.log(renderVerifyReport(r)); process.exitCode = r.ok ? 0 : 1; });
|
||||
.action(async (target, opts) => {
|
||||
const { verifyClis, renderVerifyReport } = await import('./verify.js');
|
||||
const r = await verifyClis({ builtinClis: BUILTIN_CLIS, userClis: USER_CLIS, target, smoke: opts.smoke });
|
||||
console.log(renderVerifyReport(r));
|
||||
process.exitCode = r.ok ? 0 : 1;
|
||||
});
|
||||
|
||||
program.command('explore').alias('probe').description('Explore a website: discover APIs, stores, and recommend strategies').argument('<url>').option('--site <name>').option('--goal <text>').option('--wait <s>', '', '3').option('--auto', 'Enable interactive fuzzing (simulate clicks to trigger lazy APIs)').option('--click <labels>', 'Comma-separated labels to click before fuzzing (e.g. "字幕,CC,评论")')
|
||||
.action(async (url, opts) => { const { exploreUrl, renderExploreSummary } = await import('./explore.js'); const clickLabels = opts.click ? opts.click.split(',').map((s: string) => s.trim()) : undefined; console.log(renderExploreSummary(await exploreUrl(url, { BrowserFactory: PlaywrightMCP, site: opts.site, goal: opts.goal, waitSeconds: parseFloat(opts.wait), auto: opts.auto, clickLabels }))); });
|
||||
|
||||
+2
-1
@@ -82,7 +82,8 @@ function renderCsv(data: any, opts: RenderOptions): void {
|
||||
for (const row of rows) {
|
||||
console.log(columns.map(c => {
|
||||
const v = String(row[c] ?? '');
|
||||
return v.includes(',') || v.includes('"') ? `"${v.replace(/"/g, '""')}"` : v;
|
||||
return v.includes(',') || v.includes('"') || v.includes('\n')
|
||||
? `"${v.replace(/"/g, '""')}"` : v;
|
||||
}).join(','));
|
||||
}
|
||||
}
|
||||
|
||||
+1
-8
@@ -42,18 +42,11 @@ export interface InternalCliCommand extends CliCommand {
|
||||
_lazy?: boolean;
|
||||
_modulePath?: string;
|
||||
}
|
||||
export interface CliOptions {
|
||||
export interface CliOptions extends Partial<Omit<CliCommand, 'args' | 'description'>> {
|
||||
site: string;
|
||||
name: string;
|
||||
description?: string;
|
||||
domain?: string;
|
||||
strategy?: Strategy;
|
||||
browser?: boolean;
|
||||
args?: Arg[];
|
||||
columns?: string[];
|
||||
func?: (page: IPage, kwargs: Record<string, any>, debug?: boolean) => Promise<any>;
|
||||
pipeline?: any[];
|
||||
timeoutSeconds?: number;
|
||||
}
|
||||
const _registry = new Map<string, CliCommand>();
|
||||
|
||||
|
||||
@@ -119,9 +119,7 @@ function parseLine(trimmed: string): { role: string; text: string; hasText: bool
|
||||
function stripAnnotations(content: string): string {
|
||||
// Unwrap outer single quotes first
|
||||
let line = content;
|
||||
let hadOuterQuotes = false;
|
||||
if (line.startsWith("'") && (line.endsWith("':") || line.endsWith("'"))) {
|
||||
hadOuterQuotes = true;
|
||||
if (line.endsWith("':")) {
|
||||
line = line.slice(1, -2) + ':';
|
||||
} else {
|
||||
|
||||
+19
-4
@@ -11,8 +11,22 @@ const KNOWN_STEP_NAMES = new Set([
|
||||
'intercept', 'tap',
|
||||
]);
|
||||
|
||||
export function validateClisWithTarget(dirs: string[], target?: string): any {
|
||||
const results: any[] = [];
|
||||
export interface FileValidationResult {
|
||||
path: string;
|
||||
errors: string[];
|
||||
warnings: string[];
|
||||
}
|
||||
|
||||
export interface ValidationReport {
|
||||
ok: boolean;
|
||||
results: FileValidationResult[];
|
||||
errors: number;
|
||||
warnings: number;
|
||||
files: number;
|
||||
}
|
||||
|
||||
export function validateClisWithTarget(dirs: string[], target?: string): ValidationReport {
|
||||
const results: FileValidationResult[] = [];
|
||||
let errors = 0; let warnings = 0; let files = 0;
|
||||
for (const dir of dirs) {
|
||||
if (!fs.existsSync(dir)) continue;
|
||||
@@ -35,7 +49,7 @@ export function validateClisWithTarget(dirs: string[], target?: string): any {
|
||||
return { ok: errors === 0, results, errors, warnings, files };
|
||||
}
|
||||
|
||||
function validateYamlFile(filePath: string): any {
|
||||
function validateYamlFile(filePath: string): FileValidationResult {
|
||||
const errors: string[] = []; const warnings: string[] = [];
|
||||
try {
|
||||
const raw = fs.readFileSync(filePath, 'utf-8');
|
||||
@@ -64,7 +78,7 @@ function validateYamlFile(filePath: string): any {
|
||||
return { path: filePath, errors, warnings };
|
||||
}
|
||||
|
||||
export function renderValidationReport(report: any): string {
|
||||
export function renderValidationReport(report: ValidationReport): string {
|
||||
const lines = [`opencli validate: ${report.ok ? 'PASS' : 'FAIL'}`, `Checked ${report.results.length} CLI(s) in ${report.files} file(s)`, `Errors: ${report.errors} Warnings: ${report.warnings}`];
|
||||
for (const r of report.results) {
|
||||
if (r.errors.length > 0 || r.warnings.length > 0) {
|
||||
@@ -75,3 +89,4 @@ export function renderValidationReport(report: any): string {
|
||||
}
|
||||
return lines.join('\n');
|
||||
}
|
||||
|
||||
+17
-3
@@ -6,13 +6,27 @@
|
||||
* to the `opencli test` command or CI pipelines.
|
||||
*/
|
||||
|
||||
import { validateClisWithTarget, renderValidationReport } from './validate.js';
|
||||
import { validateClisWithTarget, renderValidationReport, type ValidationReport } from './validate.js';
|
||||
|
||||
export async function verifyClis(opts: any): Promise<any> {
|
||||
export interface VerifyOptions {
|
||||
builtinClis: string;
|
||||
userClis: string;
|
||||
target?: string;
|
||||
smoke?: boolean;
|
||||
}
|
||||
|
||||
export interface VerifyReport {
|
||||
ok: boolean;
|
||||
validation: ValidationReport;
|
||||
smoke: null;
|
||||
}
|
||||
|
||||
export async function verifyClis(opts: VerifyOptions): Promise<VerifyReport> {
|
||||
const report = validateClisWithTarget([opts.builtinClis, opts.userClis], opts.target);
|
||||
return { ok: report.ok, validation: report, smoke: null };
|
||||
}
|
||||
|
||||
export function renderVerifyReport(report: any): string {
|
||||
export function renderVerifyReport(report: VerifyReport): string {
|
||||
return renderValidationReport(report.validation);
|
||||
}
|
||||
|
||||
Reference in new issue
Block a user