Agent Skills in Claude Code and Visual Studio Code: Setup, Benefits, Risks, and a Step-by-Step Example
"Copilot completed it. It works."
When you see this in a PR description, experienced reviewers put on their "failure mode" glasses. Because "it works" almost always means "it works on the happy path" — when the user provides correct input, the network responds immediately, the database is responsive, and all external services are alive and well. Production has a very different set of conditions.
The happy path is the flow where everything goes right. User provides valid input. Network is responsive. Database returns results quickly. External APIs respond with the expected format. No timeouts. No concurrent requests causing race conditions. No edge cases.
Code that works only on the happy path is a disaster waiting to happen in production. Here's why:
Look at AI-generated code like this:
async function getUserReport(userId: string) {
const user = await userService.findById(userId);
const orders = await orderService.getRecentOrders(userId);
const report = reportBuilder.build(user, orders);
return report;
}It's short, clean, and the logic appears correct. But what happens in production?
This code doesn't handle the real world where things fail constantly and in combinations.
Functions often return null when "no value is found." But the possibility must be explicitly expressed and must be handled at every call site:
// Before: returns null, but caller doesn't check
public User findByEmail(String email) {
return userRepository.findByEmail(email); // may return null, but no indication
}
// Caller doesn't check
User user = userService.findByEmail(email);
String name = user.getName(); // NPE possible if user is null
String email = user.getEmail(); // Another NPE possible
Better design makes nullability explicit in the type contract:
// After: type shows nullability explicitly
public Optional<User> findByEmail(String email) {
return userRepository.findByEmail(email);
}
// Caller must handle
Optional<User> user = userService.findByEmail(email);
String name = user.map(User::getName).orElse("Unknown");
String email = user.map(User::getEmail).orElse("");
// Or throw an exception if null is unexpected
User user = userService.findByEmail(email)
.orElseThrow(() -> new UserNotFoundException(email));
async function loadDashboard() {
const userData = await fetchUser(); // what if fails?
const orderData = await fetchOrders(); // what if fails?
const statsData = await fetchStats(); // what if fails?
renderDashboard(userData, orderData, statsData);
}If any call fails, the entire dashboard is unrendered. There's no error message to the user. Just silence or console warnings that the user never sees.
External APIs, payment gateways, email services — these fail constantly. Reviewers check whether code handles timeouts, temporary failures, rate limiting, and maintenance windows.
try {
emailService.send(userId, message);
} catch (Exception e) {
// ignore
}This is dangerous and insidious. If sending fails, absolutely no one knows. No log, no alert, no error message. The system silently fails. The user doesn't know. The ops team doesn't know. The email was silently lost.
Better approach: Log the error, and decide whether to propagate it or use a fallback strategy.
try {
validateOrder(data);
await orderRepository.save(data);
await paymentService.charge(order);
await emailService.sendConfirmation(order);
} catch (error) {
return { success: false, message: 'An error occurred.' };
}But these are fundamentally different errors:
Handling them all with one generic message hides what actually went wrong.
catch (error) {
return { error: error.getMessage() }; // DB connection info exposed!
}If the database error is `Connection refused: jdbc:postgresql://internal-db:5432/prod`, you've just exposed your entire infrastructure to an attacker.
Better approach: Log details on the server. Give a generic message to the user.
When reviewing code, systematically ask:
Race conditions often don't show in tests because tests are single-threaded. Two requests reach the same code simultaneously:
async function useCoupon(couponId, userId) {
const coupon = await couponRepository.findById(couponId);
if (coupon.usedAt !== null) throw new CouponAlreadyUsed();
// RACE CONDITION: two requests both pass the check
await couponRepository.markAsUsed(couponId, userId);
}Two requests check → both pass → both try to use the coupon → one coupon is used twice → revenue loss.
Defensive coding means trusting nothing from external systems.
□ Inputs
□ Can null or undefined appear?
□ Are empty values (empty string, empty array, 0) handled?
□ Can negative or unusually large values appear?
□ Are strings validated for length and content?
□ External dependencies
□ Is there handling for DB/API call failure?
□ Is a timeout configured?
□ Is the external response shape validated?
□ What if the service is down?
□ Concurrency
□ What happens if two requests arrive simultaneously?
□ Can intermediate state be changed elsewhere?
□ Are there race conditions?
□ State transitions
□ Is the operation idempotent (safe to retry)?
□ Can the previous state be restored if operation fails?
□ What if another process modifies state in between?
Case 1: The Null Email Bug
A service sends a welcome email to new users. The code doesn't check if email is null. Ninety-nine percent of users have email. One percent get null from a legacy import. The email service crashes on null, logs errors, and that 1% never gets their welcome email or gets an error when trying to login. This cascades into support tickets.
Case 2: The Race Condition in Inventory
A store has 1 item in stock. Two customers buy it simultaneously. The code checks inventory, finds 1, sells to both. Now the store is obligated to deliver 2 items but has 0. Returns and refunds follow. The fix requires database transactions or atomic operations — things only visible when you ask "what if two requests arrive at the same time?"
Case 3: The Silent Payment Failure
A payment API call fails (network timeout). The code has a try-catch that logs the error but doesn't fail the order. The customer thinks they paid but never actually did. Their order never ships. They get charged by their credit card company for a chargeback. Ops gets an angry call.
The fix would have been visible if someone asked "what happens when the external API fails?"
Expert reviewers develop an intuitive sense of where edge cases hide. They think in fault trees:
Develop this mental model by asking those questions consistently, across multiple PRs, until they become automatic.
Many developers write code that's correct in single-threaded scenarios but breaks with concurrency:
// Seemingly safe - but not thread-safe
public boolean useDiscount(Discount discount) {
if (discount.getUsageCount() < discount.getMaxUsages()) {
discount.setUsageCount(discount.getUsageCount() + 1);
discountRepository.save(discount);
return true;
}
return false;
}
// Two concurrent requests:
// Request 1: checks count (0), passes check
// Request 2: checks count (0), passes check ← RACE CONDITION
// Request 1: saves count (1)
// Request 2: saves count (1) ← Same value saved twice
// Discount is now used twice when max is 1Solution: Use database-level constraints:
@Lock(LockModeType.PESSIMISTIC_WRITE) // Lock at database level
public Discount findAndLockDiscount(Long discountId) {
return discountRepository.findById(discountId).orElse(null);
}
public boolean useDiscount(Discount discount) {
if (discount.getUsageCount() < discount.getMaxUsages()) {
discount.setUsageCount(discount.getUsageCount() + 1);
discountRepository.save(discount);
return true;
}
return false;
}When reviewing code, don't stop after confirming the happy path works. Ask one more time: What if the value is null? What if two requests arrive together? What if the external API fails or times out? What if the database is slow?
That one extra question prevents production incidents at 3 AM. That consistency makes you a reviewer others respect and learn from.
Expert reviewers develop intuition about failure modes. They ask the same questions across different codebases because the failure patterns are universal:
Learn these patterns. When reviewing code, consciously check each one. Over time, it becomes automatic.
Comments
Post a Comment