Add legacy-code-and-refactoring skill

[federicobartoli]

All the current skills assume new or at least tested code, but the most common thing I actually point an agent at is old untested code that nobody remembers why it works. code-simplification doesn't cover it — it explicitly wants code that's already readable and tested.

So this adds the classic Feathers playbook as a skill: characterization tests before touching anything, seams and sprout to make untestable code testable, refactor and behavior change always in separate commits, strangler fig instead of rewrites.

Wired into README, CLAUDE.md and the using-agent-skills tree. session-start-test.sh passes since I touched the meta-skill.

[nucliweb]

Strong skill, well grounded in the Feathers playbook. The characterization test workflow (write a deliberately wrong assertion, run it, copy the actual value) is concrete enough for an agent to follow without interpretation. The seam example with deps = { ... } defaults is clean and correctly shows existing callers unchanged. The commit protocol rule ("if you had to edit a test expectation, it wasn't a refactor") is precise and easy to verify. The cross-references to api-and-interface-design (Hyrum's Law) and code-simplification (Chesterton's Fence) are well placed.

One structural fix: ## Core Process should be ## Process to match the skill anatomy convention. CLAUDE.md defines the required sections as: Overview, When to Use, Process, Common Rationalizations, Red Flags, Verification.

Also: the skill is not added to the numbered lifecycle sequence in using-agent-skills, and that's the right call. Legacy work is situationally triggered, not a sequential step in a feature lifecycle. The discovery tree and Quick Reference table are the right entry points.

[federicobartoli]

Strong skill, well grounded in the Feathers playbook. The characterization test workflow (write a deliberately wrong assertion, run it, copy the actual value) is concrete enough for an agent to follow without interpretation. The seam example with deps = { ... } defaults is clean and correctly shows existing callers unchanged. The commit protocol rule ("if you had to edit a test expectation, it wasn't a refactor") is precise and easy to verify. The cross-references to api-and-interface-design (Hyrum's Law) and code-simplification (Chesterton's Fence) are well placed.

One structural fix: ## Core Process should be ## Process to match the skill anatomy convention. CLAUDE.md defines the required sections as: Overview, When to Use, Process, Common Rationalizations, Red Flags, Verification.

Also: the skill is not added to the numbered lifecycle sequence in using-agent-skills, and that's the right call. Legacy work is situationally triggered, not a sequential step in a feature lifecycle. The discovery tree and Quick Reference table are the right entry points.

Thanks @nucliweb ! Renamed to ## Process in 15c2cd6.
And glad the lifecycle omission reads as intentional, because it was!
Heads up: this and #223 both touch the skill-count lines in README and the using-agent-skills tables, so whichever merges second will need a trivial rebase (24 => 25). Happy to rebase mine once the other lands.

[addyosmani]

This is nice work. Honestly, the way you split the commits between the refactor and the behavior change is my favorite part.

I did notice one real gap in step 5, though. The "run both, log diffs" approach is a classic Scientist-style shadow pattern, but it's only safe if the legacy path doesn't have side effects, right?

If someone runs both paths on code that writes to a database, charges a card, or sends an email, they're going to double those side effects in production. It’s definitely worth adding a quick warning sentence: shadow the read paths, but for writes, either suppress the new path's side effects or compare against a dry run. We just want to make sure nobody accidentally double-charges a customer while trying to "compare outputs."

One other small thing: the text leans pretty heavily on inline references to other skills (about ten of them, like code-simplification, doubt-driven-development, api-and-interface-design, etc.).

Individually they all make sense, but stacked together mid-process, it starts to feel like a "go read six other skills first" homework assignment.

That sort of undercuts the tight, self-contained scope we want agents to maintain.

I'd suggest keeping just the most important, load-bearing ones inline (like test-driven-development for the fix and git-workflow for the commit split) and moving the rest to a quick "Related Skills" section at the very end.

Everything else looks totally merge-ready, and I appreciate you already flagging the #223 rebase for the skill count. Nice job!

[federicobartoli]

This is nice work. Honestly, the way you split the commits between the refactor and the behavior change is my favorite part.

I did notice one real gap in step 5, though. The "run both, log diffs" approach is a classic Scientist-style shadow pattern, but it's only safe if the legacy path doesn't have side effects, right?

If someone runs both paths on code that writes to a database, charges a card, or sends an email, they're going to double those side effects in production. It’s definitely worth adding a quick warning sentence: shadow the read paths, but for writes, either suppress the new path's side effects or compare against a dry run. We just want to make sure nobody accidentally double-charges a customer while trying to "compare outputs."

One other small thing: the text leans pretty heavily on inline references to other skills (about ten of them, like code-simplification, doubt-driven-development, api-and-interface-design, etc.).

Individually they all make sense, but stacked together mid-process, it starts to feel like a "go read six other skills first" homework assignment.

That sort of undercuts the tight, self-contained scope we want agents to maintain.

I'd suggest keeping just the most important, load-bearing ones inline (like test-driven-development for the fix and git-workflow for the commit split) and moving the rest to a quick "Related Skills" section at the very end.

Everything else looks totally merge-ready, and I appreciate you already flagging the #223 rebase for the skill count. Nice job!

Thanks Addy! Fixed in 27ee8b3 and a88b2f0.
You were right about the shadow comparison. The list step now says reads only, and the warning below it explains what to do for writes: run the new path in dry-run, or swap its DB/payment/mail client for one that records calls instead of executing them, then compare the recorded writes with the real ones. While I was there I also added a note about normalizing timestamps and generated IDs before diffing (otherwise everything looks like a mismatch) and about sampling the shadow traffic, since even pure reads double the load on the backend.
For the cross references I kept tdd and git-workflow inline and moved the other six to a Related Skills section at the end. I left the three in "When to Use" though, since those are exclusions telling you when not to use the skill rather than mid-process reading. Happy to move those too if you prefer.

[addyosmani]

This is exactly the fix, and then some. The shadow-writes warning covers the gap I flagged, and the two you layered on top are the ones people usually learn the hard way: normalizing timestamps and IDs before diffing, and sampling the shadow load because even reads double backend traffic. To your open question, leave the three references in When to Use. Those are exclusions ("use code-simplification / deprecation-and-migration / debugging-and-error-recovery instead"), so they route the agent to the right skill rather than assign mid-process homework. Moving them would bury a signal that belongs exactly where a reader decides whether this skill applies at all.

The Related Skills split reads better too: TDD and git-workflow stay inline where the process leans on them, and the rest sit in the footer with a one-line why each, which is more useful than the bare inline mentions were. Nothing left on my side, so this is an approve. The only tie is the skill-count line you share with #223, so whichever lands second takes the trivial rebase. Great addition.

[federicobartoli]

This is exactly the fix, and then some. The shadow-writes warning covers the gap I flagged, and the two you layered on top are the ones people usually learn the hard way: normalizing timestamps and IDs before diffing, and sampling the shadow load because even reads double backend traffic. To your open question, leave the three references in When to Use. Those are exclusions ("use code-simplification / deprecation-and-migration / debugging-and-error-recovery instead"), so they route the agent to the right skill rather than assign mid-process homework. Moving them would bury a signal that belongs exactly where a reader decides whether this skill applies at all.

The Related Skills split reads better too: TDD and git-workflow stay inline where the process leans on them, and the rest sit in the footer with a one-line why each, which is more useful than the bare inline mentions were. Nothing left on my side, so this is an approve. The only tie is the skill-count line you share with #223, so whichever lands second takes the trivial rebase. Great addition.

Thanks Addy!
Since #223 landed first, the rebase is on me: branch is now on current main with the skill count bumped to 25 in 830f1b1. That commit also fixes the README count line, which still said 24 on main after observability merged.