✨ Add playwright-browser skill with progressive disclosure #27
Conversation
Replaces bloated Playwright MCP (~16.5k tokens) with a minimal skill that costs near-zero until invoked: - SKILL.md: 68 lines vs 454 in lackeyjb/playwright-skill - run.js: Auto-install, CI detection for headless, proper async handling - API_REFERENCE.md: Advanced patterns only (network mocking, auth persistence, multi-tab, downloads, video, traces) Design decisions: - No dev server detection with prompts (use PLAYWRIGHT_BASE_URL env var) - Visible browser by default, headless in CI - Uses spawn() for script execution to properly await async code - Injected variables (BASE_URL, HEADLESS, chromium, etc.) for inline code 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
PR Review: Playwright Browser SkillOverall AssessmentThis is an excellent implementation that demonstrates best practices in progressive disclosure and token efficiency. The design philosophy is sound, the code is clean, and the prompt engineering follows the project's standards. I have a few suggestions for improvement, but this is ready to merge with minor fixes. StrengthsArchitecture & Design
Prompt Engineering
Code Quality
Issues to Address1. Security: Unquoted Path in Line 59 (run.js:59)Severity: Medium return { code: fs.readFileSync(path.resolve(args[0]), "utf8"), isFile: true };If Recommendation: This is actually fine as-is since 2. Potential Race Condition with Temp Files (run.js:136, 150)Severity: Low const tempFile = path.join(__dirname, `.temp-${Date.now()}.js`);Using Recommendation: Use a more robust approach: const tempFile = path.join(__dirname, `.temp-${Date.now()}-${process.pid}.js`);Or use the 3. Error Handling: Silent Catch in Cleanup (run.js:142-143, 156-158)Severity: Low try {
fs.unlinkSync(tempFile);
} catch {}Silent failures in cleanup can hide issues. At minimum, log errors in debug mode. Recommendation: try {
fs.unlinkSync(tempFile);
} catch (err) {
// Intentionally ignore ENOENT (file already deleted)
if (err.code !== 'ENOENT') {
console.warn('Failed to cleanup temp file:', err.message);
}
}4. Documentation: Inconsistent TerminologySeverity: Very Low In SKILL.md line 32, the inline example uses single-letter variable names ( Recommendation: Use consistent naming in examples. Either both should use full names or both should use abbreviations. I recommend full names for clarity: node $SKILL_DIR/run.js "const browser = await chromium.launch(); const page = await browser.newPage(); await page.goto('http://localhost:3000'); console.log(await page.title()); await browser.close();"5. Missing Error Context in isCompleteScript Detection (run.js:74-81)Severity: Very Low The Recommendation: This is fine for the intended use case. If issues arise, consider more robust detection, but the current implementation follows the "simple until proven wrong" principle. Performance Considerations
Security Considerations
Testing RecommendationsWhile the PR description mentions code review by agents, I recommend manual testing for:
Nitpicks
VerdictApprove with minor fixes recommended This PR demonstrates excellent engineering and follows the project's prompt engineering standards. The issues identified are minor and don't block merging. I particularly appreciate the progressive disclosure pattern and the attention to token efficiency. The only must-fix is making temp file names more robust (add PID). The rest are nice-to-haves. Great work on this implementation! |
Pulled best practices from lackeyjb/playwright-skill: - cleanupOldTempFiles() for disk hygiene - Custom headers support (PW_HEADER_NAME, PW_EXTRA_HEADERS) - CI sandbox args (--no-sandbox, --disable-setuid-sandbox) - Better error output formatting with emojis Changed headless default: - Now headless by default (less intrusive) - PLAYWRIGHT_HEADED=true for visible browser - SKILL.md guides Claude on when to use headed mode (user says "show me", "watch", "debug", etc.) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Remove prescriptive word list. State the goal, trust the model. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Code Review: Playwright Browser SkillThis is an excellent implementation of a progressive disclosure skill. The code quality is high, and the design decisions are well-reasoned. Below are my findings across different review categories. ✅ StrengthsDesign & Architecture
Code Quality
Prompt EngineeringThe SKILL.md follows prompt engineering best practices well:
🔍 Issues Found🔴 Critical: Command Injection Vulnerability (run.js:91-99)Location: Issue: The inline code path uses if (args.length > 0) {
console.log("⚡ Executing inline code");
return args.join(" "); // ⚠️ Vulnerability
}Attack vector: If the shell splits arguments unexpectedly (e.g., unquoted spaces, semicolons, backticks), the code could be malformed or execute unintended commands. Example: node run.js console.log('test'); malicious.code()
# args = ['console.log(\'test\');', 'malicious.code()']
# joined = "console.log('test'); malicious.code()"Recommendation: Since the first argument check (line 91) already handles file paths, the remaining arguments should be treated as a single inline code string. Consider: if (args.length > 0 && !fs.existsSync(args[0])) {
console.log("⚡ Executing inline code");
return args.join(" "); // This is actually fine since we expect the shell to have already split it
}Actually, upon further review, this might be working as intended - the shell's argument parsing is being trusted here. However, the documentation should clarify that inline code should be properly quoted when calling the script. Severity: Medium (requires specific invocation patterns to exploit, but should be documented) 🟡 Medium: Race Condition in Temp File Naming (run.js:205)Location: Issue: Using const tempFile = path.join(__dirname, `.temp-${Date.now()}.js`);Recommendation: Add process PID or use a random component: const tempFile = path.join(__dirname, `.temp-${Date.now()}-${process.pid}.js`);Severity: Low (very unlikely in practice, but easy to fix) 🟡 Medium: Incomplete Script Detection Heuristic (run.js:115-122)Location: Issue: The function isCompleteScript(code) {
const hasRequire = code.includes("require(");
const hasAsyncWrapper =
code.includes("(async () =>") ||
code.includes("(async()=>") ||
code.includes("(async function");
return hasRequire && hasAsyncWrapper;
}Problems:
Recommendation: Either:
Severity: Medium (could cause unexpected behavior, but rare) 🟡 Medium: Missing Browser Cleanup DocumentationLocation: Issue: None of the examples explicitly show browser cleanup with Example (API_REFERENCE.md:100-113): const context = await browser.newContext({
recordVideo: {
dir: '/tmp/videos/',
size: { width: 1280, height: 720 }
}
});
const page = await context.newPage();
// ... do things ...
await page.close();
const videoPath = await page.video().path();
console.log('Video saved:', videoPath);
// ⚠️ Missing: await context.close(); await browser.close();Impact: Memory leaks and zombie browser processes, especially problematic in CI. Recommendation: Add cleanup to all examples, or add a "Cleanup" section showing proper teardown. Severity: Medium (can cause real problems in production use) 🟢 Minor: Silent Error Handling (run.js:79-85)Location: Multiple locations with empty catch blocks Issue: Several places have silent error handling that might hide bugs: tempFiles.forEach((file) => {
try {
fs.unlinkSync(path.join(__dirname, file));
} catch {
// Ignore - file might be in use
}
});Recommendation: At minimum, log to stderr in debug mode: } catch (e) {
if (process.env.DEBUG) {
console.error(`Failed to cleanup ${file}:`, e.message);
}
}Severity: Low (intentional design choice, but could hinder debugging) 🟢 Minor: API_REFERENCE.md Line 139 - Outdated Device NameLocation: Issue: The example uses Recommendation: Either verify the device name is correct for Playwright 1.49.0, or use a more common device like Severity: Low (example code that might not work as shown) 🟢 Minor: Inconsistent Error Message FormatLocation: Issue: Error messages use inconsistent emoji prefixes:
Some include the error object, some just the message, some have different prefix patterns. Recommendation: Standardize on a consistent format throughout. Severity: Low (cosmetic, but consistency helps debugging) 🎯 RecommendationsHigh Priority
Medium Priority
Low Priority
🔒 Security Review✅ Good Security Practices
|
PR Review: Playwright Browser SkillThis is a well-designed implementation that follows progressive disclosure principles. The code quality is high, and the architecture is sound. Below are my findings organized by category. ✅ StrengthsArchitecture & Design:
Code Quality:
LLM-Friendly Design:
🔍 Code Quality Observationsrun.js:115-122 - // Would be detected as "complete" and not wrapped:
const code = `
const pw = require('playwright');
(async () => { /* user's async code */ })();
`;
// Would NOT be detected and get double-wrapped:
const code = `
import { chromium } from 'playwright';
(async () => { /* user's async code */ })();
`;The run.js:88-113 - Argument parsing: if (args.length > 0 && fs.existsSync(args[0])) {
// Execute as file
}
if (args.length > 0) {
// Execute as inline - THIS COULD MATCH A FAILED FILE PATH
return args.join(" ");
}If someone passes a file path that doesn't exist, it falls through to inline code execution, which would try to execute the file path as JavaScript. This would fail gracefully with an error, but a clearer error message would be better: if (args.length > 0) {
if (fs.existsSync(args[0])) {
const filePath = path.resolve(args[0]);
console.log(`📄 Executing: ${filePath}`);
return fs.readFileSync(filePath, "utf8");
}
// If it looks like a file path but doesn't exist, warn the user
if (args[0].endsWith('.js') || args[0].includes('/')) {
console.warn(`⚠️ Warning: '${args[0]}' looks like a file path but doesn't exist. Treating as inline code.`);
}
console.log("⚡ Executing inline code");
return args.join(" ");
}run.js:124-146 - try {
const parsed = JSON.parse(headersJson);
if (typeof parsed === 'object' && parsed !== null && !Array.isArray(parsed)) {
return parsed;
}
console.warn('⚠️ PW_EXTRA_HEADERS is not a valid object, ignoring');
} catch (e) {
console.warn('⚠️ PW_EXTRA_HEADERS is not valid JSON, ignoring:', e.message);
}🛡️ Security ConsiderationsLow Risk - Code Execution by Design:
This is appropriate for a developer tool. No changes needed. Temporary File Handling (run.js:205, 217-218): const tempFile = path.join(__dirname, `.temp-${Date.now()}-${Math.random().toString(36).slice(2, 8)}.js`);This is a minor issue and probably not worth changing unless concurrent execution is a goal. Auto-install (run.js:53-68): 🎯 Best Practices & ConventionsFollows Repository Standards:
SKILL.md Design: Use headed when user wants to see the browser. You know when that is.This is excellent. It's goal-focused rather than prescriptive (no word lists), trusting the model's judgment. 📊 Performance ConsiderationsToken Efficiency:
Runtime Performance:
🧪 Test CoverageMissing Tests:
While tests aren't blocking for a skill (this is developer tooling), they would increase confidence, especially around the script detection heuristics. 💡 Suggestions (Optional Improvements)
🎯 VerdictRecommendation: Approve with minor suggestions This is high-quality work that achieves its design goals. The code is clean, follows project conventions, and implements progressive disclosure effectively. The few issues identified are minor edge cases that won't affect normal usage. The lack of automated tests is the main gap, but for a developer tool like this, manual testing may be sufficient. If this skill becomes widely used, adding tests would be valuable. Blocking issues: None Great work on following the prompt engineering principles and keeping the LLM-facing interface minimal and goal-focused! 🎭 |
- Warn if .js file path doesn't exist (treating as inline code) - Warn if PW_EXTRA_HEADERS is invalid JSON 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
|
Thanks for the thorough review! Addressed the two quick wins:
Deferring for later:
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| const path = require("path"); | ||
| const { execSync, spawn } = require("child_process"); | ||
|
|
||
| process.chdir(__dirname); |
There was a problem hiding this comment.
Relative file paths resolve incorrectly after chdir
The process.chdir(__dirname) at line 25 executes before getCodeToExecute() processes file arguments. When a user passes a relative path like ./my-test.js, the fs.existsSync() and path.resolve() calls resolve it relative to the skill directory instead of the caller's working directory. This causes valid script files to not be found, triggering the fallback that treats the file path as inline JavaScript code, which then fails to execute.
Additional Locations (1)
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
Code Review: Playwright Browser SkillSummaryExcellent implementation of a minimal, progressive disclosure skill for Playwright automation. The design achieves its stated goal of replacing a bloated MCP with a near-zero token cost until invoked. The code is well-structured, handles edge cases thoughtfully, and follows the project's prompt engineering principles. Strengths1. Progressive Disclosure Pattern ✅The skill perfectly demonstrates the progressive disclosure pattern:
2. Excellent Prompt EngineeringThe SKILL.md follows prompt-engineering best practices:
3. Robust Error Handling
4. CI/CD Awareness
Issues FoundCode QualityMinor: Race Condition in Temp File CleanupLocation: run.js:212-226 The temp file cleanup in the finally block could fail if the script is still executing when cleanup runs. While unlikely due to the await, there's a theoretical race. Suggestion: Add a small delay or verify the child process has exited: finally {
// Small delay to ensure spawn process has fully released file
await new Promise(resolve => setTimeout(resolve, 100));
try {
fs.unlinkSync(tempFile);
} catch {}
}Minor: String Interpolation SecurityLocation: run.js:170 User-provided code is injected directly into the wrapped script. While this is the intended functionality, there's no sanitization of the code string. Impact: Low risk since this is a local execution tool, but malicious inline code could break the wrapper or inject unexpected behavior. Recommendation: Document this behavior clearly or add a warning if code contains template literal escape sequences. Minor: Inconsistent Error OutputLocation: run.js:172-174 vs run.js:230-231 Error handling uses different formats:
Suggestion: Use consistent terminology ("Error" vs "Fatal") or document when each applies. Best PracticesLow Priority: Missing Input ValidationLocation: run.js:88-117 The Suggestion: Add validation: if (!process.stdin.isTTY) {
console.log("📥 Reading from stdin");
const code = fs.readFileSync(0, "utf8");
if (!code.trim()) {
console.error("❌ No code provided via stdin");
process.exit(1);
}
return code;
}PerformanceOptimization: Redundant CI DetectionLocation: run.js:27-35 and run.js:157
const IS_CI = isCI();
const CI_ARGS = IS_CI ? "['--no-sandbox', '--disable-setuid-sandbox']" : "[]";Minor impact, but improves code clarity. SecurityMedium: Command Injection via Environment VariablesLocation: run.js:128-153 The Example Attack: PW_HEADER_NAME='x", "evil": process.exit(1), "y":"z' node run.js "console.log('test')"Impact: Local execution context only, but still a concern. Recommendation: Add validation: function getExtraHeaders() {
const headerName = process.env.PW_HEADER_NAME;
const headerValue = process.env.PW_HEADER_VALUE;
if (headerName && headerValue) {
// Validate header name follows HTTP spec
if (!/^[a-zA-Z0-9-]+$/.test(headerName)) {
console.warn('⚠️ PW_HEADER_NAME contains invalid characters, ignoring');
return null;
}
return { [headerName]: headerValue };
}
// ... rest
}Testing CoverageThe PR description mentions:
Missing:
Recommendation: Add a
Design Decisions Review
DocumentationSKILL.md
API_REFERENCE.md
Minor suggestion: Add a "When to use this reference" section at the top to guide users. RecommendationsMust Fix
Should Fix
Nice to Have
Overall AssessmentQuality: High This is a well-designed skill that achieves its goals elegantly. The progressive disclosure pattern and minimal token cost are exactly what this project needs. With the security fix and basic test coverage, this would be production-ready. The design decisions are sound and well-justified. The shift from headed to headless by default based on review feedback shows good iteration. Recommendation: Merge after addressing the header validation security issue. Testing can be added in a follow-up PR if needed. |
| } catch { | ||
| // Ignore directory read errors | ||
| } | ||
| } |
There was a problem hiding this comment.
Race condition deletes temp files from concurrent instances
The cleanupOldTempFiles function deletes ALL .temp-*.js files despite its name suggesting it only cleans old files. When multiple instances of run.js execute concurrently, one instance can delete another's temp file before its child process opens it. The sequence is: Instance A creates temp file and spawns child → Instance B starts, runs cleanup, deletes A's file → A's child fails to read the deleted file. The try-catch handles files currently open but not the race window between file creation and the spawned child opening it.
Additional Locations (1)
| } | ||
|
|
||
| const code = getCodeToExecute(); | ||
| const tempFile = path.join(__dirname, `.temp-${Date.now()}.js`); |
There was a problem hiding this comment.
Temp file name collision when processes start same millisecond
The temp file name uses only Date.now() for uniqueness, which has millisecond precision. If two instances of run.js start within the same millisecond (possible with automated tooling or parallel CI), both create the same temp file path. Process A writes its code, then Process B overwrites it with different code, then Process A executes Process B's code instead of its own. Adding process.pid or a random component to the filename would prevent this collision.
- Description now starts with "Use when..." per CLAUDE.md guidelines - Bump plugin version 8.5.0 → 8.6.0 for new playwright-browser skill 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Code Review: Playwright Browser SkillThis is a well-designed implementation that follows the progressive disclosure pattern effectively. The code quality is high overall, with good error handling and thoughtful design decisions. Here are my detailed findings: ✅ StrengthsArchitecture & Design:
Code Quality:
Documentation:
🔍 Issues & Concerns1. Security: Arbitrary Code Execution (High Severity)Location: The skill executes user-provided code directly without sanitization. While this is inherent to Playwright automation, there are safety improvements to consider: // Current implementation injects code directly
${code}Concerns:
Recommendations:
2. Race Condition in Temp File Creation (Medium Severity)Location: const tempFile = path.join(__dirname, `.temp-${Date.now()}.js`);Issue: If two instances run in the same millisecond, they'll use the same filename, causing conflicts. Fix: const tempFile = path.join(__dirname, `.temp-${Date.now()}-${Math.random().toString(36).substring(7)}.js`);Or use Node's built-in temp file creation: const crypto = require('crypto');
const tempFile = path.join(__dirname, `.temp-${crypto.randomBytes(8).toString('hex')}.js`);3. Incomplete isCompleteScript Detection (Medium Severity)Location: function isCompleteScript(code) {
const hasRequire = code.includes("require(");
const hasAsyncWrapper =
code.includes("(async () =>") ||
code.includes("(async()=>") ||
code.includes("(async function");
return hasRequire && hasAsyncWrapper;
}Issues:
Suggestions:
4. Missing Error Context in runScript (Low-Medium Severity)Location: child.on("close", (code) => {
if (code === 0) {
resolve();
} else {
reject(new Error(`Script exited with code ${code}`));
}
});Issue: When scripts fail, users only see exit code, not what failed. Improvement: child.on("close", (code) => {
if (code === 0) {
resolve();
} else {
reject(new Error(`Script exited with code ${code}. Check output above for details.`));
}
});Even better: Capture stderr separately for better error reporting. 5. Cleanup Race Condition (Low Severity)Location: } finally {
try {
fs.unlinkSync(tempFile);
} catch {}
}Issue: Silent catch swallows all errors, including permission issues that users should know about. Better: } finally {
try {
fs.unlinkSync(tempFile);
} catch (e) {
if (e.code !== 'ENOENT') {
console.warn(`⚠️ Failed to cleanup temp file: ${e.message}`);
}
}
}6. Undocumented EXTRA_HEADERS BehaviorLocation: SKILL.md mentions
Add to SKILL.md: - `EXTRA_HEADERS` - Custom headers object (null if not set)
Example: PW_EXTRA_HEADERS='{"Authorization":"Bearer token"}'🎯 Best Practices & StyleMinor Improvements:
const TEMP_FILE_PREFIX = '.temp-';
const TEMP_FILE_EXTENSION = '.js';
"engines": {
"node": ">=14.8.0"
}🔒 Security Considerations
🧪 Test CoverageMissing: No automated tests for this skill. Recommendations:
Example test structure: // test/playwright-browser.test.js
describe('run.js', () => {
it('should execute inline code');
it('should execute file-based scripts');
it('should handle stdin input');
it('should cleanup temp files');
it('should warn on invalid PW_EXTRA_HEADERS');
});📊 Performance✅ Good:
Considerations:
📋 SummaryVerdict: ✅ Approve with minor recommendations This is a solid implementation that achieves its design goals effectively. The identified issues are mostly minor, with the exception of the security considerations around arbitrary code execution - which is inherent to the tool's purpose but should be documented. Priority Fixes:
Nice-to-Haves:
The progressive disclosure pattern is excellent and this will be a valuable addition to the plugin ecosystem. Great work on keeping it minimal while still being feature-complete! 🎭 |
| code.includes("(async()=>") || | ||
| code.includes("(async function"); | ||
| return hasRequire && hasAsyncWrapper; | ||
| } |
There was a problem hiding this comment.
Pattern matching in comments breaks variable injection
The isCompleteScript function uses String.includes() to detect whether code needs the injected variable wrapper. This matches patterns like require( and (async () => anywhere in the string, including inside comments or string literals. If user code contains both patterns in comments (e.g., // use require('playwright') and // wrap with (async () => ...)), the function incorrectly returns true, causing the code to run without the injected chromium, BASE_URL, HEADLESS, and other variables. This results in confusing "X is not defined" runtime errors.
| const headerValue = process.env.PW_HEADER_VALUE; | ||
| if (headerName && headerValue) { | ||
| return { [headerName]: headerValue }; | ||
| } |
There was a problem hiding this comment.
Partial header environment variables silently ignored without warning
When PW_HEADER_NAME is set without PW_HEADER_VALUE (or vice versa), the header configuration is silently ignored without any warning. This is inconsistent with the behavior for PW_EXTRA_HEADERS, which logs a warning when the JSON is invalid. A user who sets only PW_HEADER_NAME expecting a header to be sent may experience silent authentication or request failures, making the issue difficult to debug.
Summary
Changes
New skill at
plugins/core/skills/playwright-browser/:SKILL.md- Minimal execution pattern, no examples Claude already knows from trainingrun.js- Universal executor with auto-install, CI detection, proper async handlingpackage.json- Playwright dependencyAPI_REFERENCE.md- Advanced patterns only (network mocking, auth persistence, multi-tab, downloads, video, traces)Design Decisions
PLAYWRIGHT_BASE_URLenv var insteadBASE_URL,HEADLESS,chromium, etc. available without boilerplateTesting
🤖 Generated with Claude Code