Port Batch 1 upstream defect fixes, with the divergence review behind them - #23
Open
lucianghinda wants to merge 19 commits into
Open
Port Batch 1 upstream defect fixes, with the divergence review behind them#23lucianghinda wants to merge 19 commits into
lucianghinda wants to merge 19 commits into
Conversation
This fork diverged from obra/superpowers at 8ea3981 (2026-03-19). Since then upstream landed 291 commits across 100 merged PRs and reached v6.2.0 while this fork went its own way to v7.4.0. A rebase is not possible: 11 of the 14 shared skills moved on both sides. The review documents what is worth porting and why, with each rationale quoted from the merged upstream PR rather than inferred. Upstream's PR template requires "What problem are you trying to solve?" and "What alternatives did you consider?", so the reasoning is available first-hand. Two reviewers covered the delta: one for PR rationale depth, one for commit-ledger completeness. Every claim about this fork's own state was verified by grep against the working tree before being written down. The handoff records the agreed integration order as four batches and the open policy questions that still need a decision. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Codex treats a manifest with no `hooks` field differently from one with an
explicit empty object. With the field absent, Codex auto-discovers the
repository-root hooks/hooks.json — which is Claude Code's SessionStart
registration — and tries to run a Claude-shaped hook it should ignore.
Only `hooks: {}` suppresses that discovery. An empty array does not match
the schema Codex expects, and deleting the hook files would break Claude
Code, so the empty object is the only cross-harness-safe fix.
Ported from obra#1897 (released upstream as v6.1.1).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`find .` emits every path with a `./` prefix, so `-path "src/**/*.test.ts"` matched nothing. The script then ran `wc -l` on an empty string, which returns 1, printed "Found 1 test files", looped zero times, and exited 0 with "No polluter found - all tests clean!". A debugging tool that silently reports success is worse than one that errors. Two further matching gaps are fixed at the same time: a pattern the user writes with a leading `./` no longer double-prefixes, and because find's `-path` cannot match `**/` against zero directory levels, the pattern is also tried with `**/` collapsed so files sitting directly under the base directory (src/top.test.ts) are no longer skipped. Verified against a toy repo: the old script reproduces the silent no-op, the new one counts all three test files, bisects to the polluter, honestly reports 0 when nothing matches, and accepts either pattern spelling. Ported from obra#2011, which fixed upstream issue obra#2008. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude Code scans tool result bodies for a literal keyword that requests deeper reasoning. This skill contained that exact token, so every session that loaded the skill made the harness inject the reminder as though the user had typed it — silently switching the session into extended thinking, raising token cost and shifting model behaviour without consent. Hyphenating the word breaks the contiguous letter sequence the scanner matches while keeping the instruction readable for the agent. Ported from obra#1558, which fixed upstream issue obra#1283. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both skills pointed at a sibling reference file with an `@filename.md` prefix. That form is not a resolvable markdown link, so an agent can treat the filename as literal text or guess at the path instead of opening the file. The referenced documents exist next to each SKILL.md, so a plain relative link is unambiguous. Ported from obra#1532 (upstream issue obra#1529) and obra#631. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three classes of text that only made sense in the original author's context: Private shorthand: receiving-code-review told the agent to signal discomfort with a personal catchphrase. The underlying behaviour is worth keeping, so it becomes direct guidance — name the tension, then raise the issue — rather than being deleted outright. Hardcoded home directories: `/Users/jesse/...` appeared in four example paths. These break for anyone on a different machine and leak someone else's directory layout into a published plugin. Ambiguous skill names: the bootstrap referred to "debugging" in three places while the skill is registered as systematic-debugging, which was reported upstream as confusing agents about whether a skill by that name existed. Ported from obra#1531, obra#1122, and obra#1601. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Batch 1 of the upstream integration shipped in the same session that created this handoff, so it never needed to be resumed. Update it with what actually landed — five commits, the SHAs, and how each was verified — then move it to _archive/ alongside the earlier handoffs. Notable additions for whoever picks up Batch 2: the opencode test runner is already red on main for an unrelated reason (it copies a lib/ directory this repo does not have), so a failing suite there is not a signal that a port broke something. The receiving-code-review wholesale port now has to account for the Circle K fix landing here first. The frontmatter follows the existing archive convention (restored: true plus restored_at) so handoff-list keeps reporting it correctly. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Batch 1 is on this branch; Batches 2-4 are not started. Record where to resume, which decisions still govern the remaining work, and the environment traps worth not rediscovering. Deliberately shorter than the archived handoff it succeeds: the deep rationale now lives in the committed review document, and a handoff that restates its own reference material only drifts from it. Key Decisions keeps just the calls that change what the next session does. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Marked restored and moved to _archive so handoff-list stops offering it as pending work. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`experimental.chat.messages.transform` runs on every agent step, and it called getBootstrapContent() unconditionally at the top — a full existsSync, readFileSync and frontmatter regex over using-superpowers/SKILL.md — before reaching the guard that returns early once the bootstrap is already in the first user message. A fifty-step session paid for fifty reads and used one. The file cannot change while the process lives, so the result is memoized at module rather than closure scope: the plugin factory may run more than once per process, and a cache inside it would reset with it. The sentinel is `undefined` for "not yet attempted" so that `null` can mean "attempted, file missing" and stay cached — otherwise a genuinely absent SKILL.md would be rediscovered on every single step, which is exactly the case where retrying is pointless. Verified by counting reads of SKILL.md across three transforms with a patched fs: three before, one after, with the bootstrap still injected on every step. The handoff and compaction hooks this fork adds are untouched. Ported from the resolution of obra#1202 (released upstream as v5.1.0). Upstream's own follow-up work on OPENCODE_CONFIG_DIR is out of scope here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ills
Both files were byte-identical to the upstream merge base apart from this
fork's own Circle K replacement, so upstream's copies apply wholesale.
Confirmed before overwriting: upstream landed the same replacement text for
that line, so the diff carries no regression and the guidance survives.
dispatching-parallel-agents loses more than length. Its dispatch example was
fenced as TypeScript and called `Task("...")`, an API no harness actually
exposes — a skill demonstrating a function that does not exist teaches
agents to call it. The block is now harness-neutral notation, and the rule
that decides the outcome is stated in words: multiple dispatch calls in one
response run in parallel, one per response runs sequentially. That was
implicit in the old example and easy to miss.
The rest is content that costs context on every load without changing
behaviour: a "Key Benefits" list selling a skill already being read, a
"Real-World Impact" anecdote, and a "Bottom Line" recap of rules stated
earlier at their decision points.
verification-before-completion is deliberately left alone. Upstream cut its
"claiming work is complete without verification is dishonesty" line under an
eval they recorded as inconclusive and low-confidence.
Ported from obra#1934.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
decodeFrame took the payload length straight from the client's 64-bit frame
header and carried it, unchecked, to Buffer.alloc(payloadLen). A local page
or extension could declare any size it liked. With the bytes actually
supplied the server allocated them; with only the header sent it returned
null and waited, while socket.on('data') kept doing Buffer.concat on a
buffer that would never complete a frame.
Oversized frames are now rejected from the header alone, before any
allocation. The 64-bit length is compared as a BigInt rather than after
Number() narrowing, because above 2^53 that conversion is lossy and the
comparison would be judging an already-rounded value.
The second check after both length branches is unreachable today — the
16-bit branch tops out at 65535, far under the 10 MiB cap — but it states
the invariant immediately above the allocation it protects and keeps the
guard correct if the cap is ever lowered.
No call-site change was needed: the data handler already catches throws from
decodeFrame, sends a CLOSE frame and drops the client.
Verified against the pre-fix decoder: it accepts a 2^63 declared length and
allocates for a cap+1 frame, both of which now throw. Exactly-at-the-cap is
still accepted, small frames decode unchanged, and the existing unmasked
frame guard still fires.
Ported from obra#1555, which fixed upstream issue obra#1446. Taken
as a standalone patch rather than as part of the wholesale scripts/ port
because the rest of the companion hardening (obra#1720) lands on its own branch.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Brings upstream's regression test for the cap added in the previous commit, per the integration rule that accepted runtime changes travel with their tests. Purely additive — the rest of ws-protocol.test.js was already identical to upstream. Confirmed load-bearing: the test fails against the pre-cap decoder and passes against the fixed one. Suite goes 31/31 to 32/32. Ported from obra#1555. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Option 1 could not complete in a worktree, which is the exact setup this fork's using-git-worktrees skill creates. Two ordering defects, both reproduced in a toy repo: Step 5 detected the worktree with `git worktree list | grep $(git branch --show-current)`, but Options 1 and 4 had already run `git checkout <base-branch>`. The query was correct; the moment was wrong. It read `main`, matched the *main* worktree, and pointed cleanup at the wrong target. The path is now captured in Step 2 while still inside the workspace, along with git-dir/git-common-dir, and Step 6 uses those saved values. Option 1 then ran `git branch -d` before cleanup ever happened. Git refuses to delete a branch checked out in a worktree — "cannot delete branch 'x' used by worktree at '...'" — so the option failed end to end. Worktree removal now runs first, and the dependency is stated where the command is. The file also contradicted itself: Step 5 cleaned up for "Options 1, 2, 4" while the Quick Reference, Common Mistakes and Red Flags all said Option 2 keeps its worktree. Option 2 keeps it — PR feedback gets fixed there. Beyond the bugs, this adopts upstream's shape: detached-HEAD gets a reduced menu with no merge, and discard comes off the numbered menu entirely. Any placement on a normal completion menu advertises destruction next to merging; it now happens only on an explicit request. Cleanup is provenance-checked rather than unconditional, and refuses to `--force` past a dirty worktree, since that may hold uncommitted work nobody has seen. The provenance check covers all three locations using-git-worktrees actually creates, including ~/.config/superpowers/worktrees/<project>/. Upstream's version tests only .worktrees/ and worktrees/, so taking it verbatim would have silently orphaned every globally-located worktree in this fork. `gh pr create` is kept deliberately — this fork's workflow is GitHub-based — with a fallback sentence for other forges rather than upstream's full removal. Ported from obra#1933, which supersedes obra#1665. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
/brainstorm, /write-plan and /execute-plan carried no behaviour — each only told the user the command was deprecated and would go away "in the next major release". They were deprecated in 5.0.0 and this fork is on 7.4.0, so two major releases have come and gone. Named-command registries are local to each user's harness install, so there is no public API to break by removing them; leaving a stub for backward-compatibility would just keep printing a notice about a removal that already happened. .cursor-plugin/plugin.json declared "commands": "./commands/" and was the only manifest to reference the directory, so it drops that key in the same commit rather than pointing at a path that no longer exists. All three manifests re-parse as valid JSON and every remaining "./" path they declare resolves. Ported from obra#1188. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
agents/code-reviewer.md and skills/requesting-code-review/code-reviewer.md were two independently drifting definitions of the same reviewer, and the drift was severe: the agent was a 48-line generic persona (SOLID, type safety, prose sections), the skill template a 153-line Ruby/Rails contract (Sandi Metz limits, thin controllers, Minitest over RSpec, fixtures over factories, bin/rails test) with placeholders, an explicit git range and a required output shape. The dispatch line combined them incoherently — "Task tool with superpowers-ruby:code-reviewer type, fill template at code-reviewer.md" handed the reviewer the generic persona as its system prompt and the Ruby template as its message, leaving two conflicting instruction sets to be resolved by whichever the model weighted higher. A prompt template is versioned by this repo; a named agent lives in each user's harness registry. Only the template can be kept correct here, and only the template works on Codex, which has no named agent registry at all — codex-tools.md carried a bespoke "find the agent's prompt file" workaround purely to paper over that. Salvaged from the persona before deleting it: judging whether a deviation from the plan is a justified improvement or a problematic departure, and routing plan-level problems back as plan changes instead of filing them against code that correctly implements a bad plan. Neither was in the template. Updated every dispatch site so nothing points at the removed registry entry: requesting-code-review/SKILL.md, subagent-driven-development's code-quality-reviewer-prompt.md, and the Codex and Copilot tool references. .cursor-plugin/plugin.json drops its "agents" key; all three manifests parse and every "./" path they declare resolves. Removing a named agent breaks no public API — named-agent registries are local to each user's harness install. Ported from obra#1299. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The skill presented prohibition + rationalization table + red flags as the universal hardening form. It works — for discipline failures, where an agent knows the rule and skips it under pressure. It backfires on shaping failures, where the agent complies but produces the wrong shape: under a competing incentive, agents negotiate with "don't X" and emit more of the unwanted content than with no guidance at all. Authors following this skill reached for the prohibition in every composition-shaping sample, because the skill taught the tool without teaching tool selection. Adds a failure-type-to-form table, two rules that hold whichever form you pick, a scope note on Bulletproofing pointing shaping problems elsewhere, a micro-test method for verifying wording before spending full pressure scenarios, and two checklist lines. Both rules say the same counterintuitive thing: qualifying words weaken a rule rather than refine it. A nuance clause reopens the negotiation, and an exemption clause does not actually scope — "this limit doesn't apply to code blocks" still suppresses code blocks. The fix is restructuring so the rule cannot reach the exempt part. Purely additive: 33 insertions, no deletions. The Iron Law, red flags, rationalization tables and partner language are untouched, and so is the relative-link fix from 2d426b4. Taken from upstream's current text rather than the original PR body. obra#1741 quoted specific per-arm scores; upstream has since replaced those with the qualitative result plus "micro-test your own case rather than assuming", which is the more defensible claim to ship. Ported from obra#1741. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Four separable hardening fixes, each with a first-hand upstream failure report. Upstream's single-reviewer consolidation is deliberately NOT taken — its author publicly corrected their own catch-rate claim (two reviewers 4/5 against the new design's 3/5) and a user reported doubled token usage. The spec and code-quality reviewers both stay. The fix loop said "Repeat until approved" with no breaker. Reviewers are nondeterministic, so approval is not a fixed point the loop converges to — each pass samples a different subset of findings and can keep discovering new ones indefinitely. Now five rounds maximum: rounds 1-3 resume the original implementer whose context is intact, rounds 4-5 dispatch a fresh one on a more capable model, and an exhausted breaker forces the controller to adjudicate each open finding on the record — BLOCKED to the human partner if any is load-bearing. Three failed resumes is evidence about the agent rather than the task, which is why round 4 changes context and capability together. (obra#1998) That section also contradicted itself about who owns corrections: the fix loop said the same subagent fixes, the block six lines below said dispatch a fix subagent. Resolved in favour of the round schedule, with the outright-failure path scoped to BLOCKED/NEEDS_CONTEXT where it belongs. An implementer could claim TDD while giving the controller no proof the test failed first, so the report contract now requires RED and GREEN commands with their output. "I followed TDD" is not evidence. (obra#1065) The spec reviewer was told to "read the implementation code" with no bounded evidence, so reviewers re-read the repository. It now gets the task's diff range, with out-of-diff inspection allowed only against a named risk that must appear in the report. Upstream also suggested solving this by routing reviewers to a cheaper model; that model-policy assumption is not taken. (obra#1538) Reviewers attempting implementation-side repository operations caused detached-HEAD failures, so both reviewer paths are now explicitly read-only on the checkout. (obra#1543) Upstream's versions of these are entangled with its durable SDD workspace (report files, ledger, review-package script). This fork never adopted that workspace, so none of it comes along — verified absent. Ported from obra#1065, obra#1538, obra#1543 and obra#1998. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ndexes Bottom-of-file Integration sections were mostly duplicate pointers to skills already named at the decision point, and some had gone stale — finishing-a-development-branch claimed it was called by "subagent-driven-development (Step 7)" and "executing-plans (Step 5)", and neither skill has ever had those steps. But some entries were sole carriers: the only place a real requirement was stated. Deleting the indexes first would have dropped them silently, so each moved to its point of use before anything was removed. - executing-plans and subagent-driven-development each carried "REQUIRED: Set up isolated workspace before starting" in the index and nowhere else. Now a Step 0 and a Setup section respectively, where an agent reads them before dispatching work. - systematic-debugging's handoff to verification-before-completion is now in Phase 4 next to "Verify Fix", instead of a Related-skills list after the process ends. - Both plan-executing skills pointed at writing-plans only from the index; that provenance moved to where the plan is read. Auditing every remaining entry mechanically against the whole skill directory — not just SKILL.md — caught a fourth sole carrier the review had not flagged. The implementer prompt said "following TDD if task says to", which reads as though the requirement is carried, but the pointer to superpowers-ruby:test-driven-development existed only in the index. Naming a concept in prose is not the same as a reference an agent can act on; the prompt now names the skill. Verified after pruning: every skill previously listed in a removed index is still referenced somewhere in the referring skill's directory. The using-git-worktrees and using-sqlite-worktrees indexes are left alone. Their entries encode REQUIRED delegation semantics between two Rails-specific fork skills, and are worth stating twice. Ported from obra#1932. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
obra/superpowersthat are still live in this fork: a Codex manifest gap, a silently-failing debugging script, a harness keyword trap, two unresolvable doc links, and leftover personal references.docs/plans/2026-08-04-upstream-divergence-review.md— a triage of the full 291-commit / 100-PR upstream delta since this fork diverged at8ea3981(2026-03-19), with each recommendation's rationale quoted from the merged upstream PR rather than inferred.Why
find-polluter.shreported "all tests clean" without running a single test, and the Codex manifest gap makes Codex load a Claude-shaped hook it should ignore.Commit-by-commit details
docs(upstream): Add divergence review and integration handoffWhat:
Why:
Notes:
fix(codex): Suppress Claude hook auto-discovery in the Codex manifestWhat:
"hooks": {}to.codex-plugin/plugin.json.Why:
hooksfield from an explicit empty object. With the field absent it auto-discovers the repo-roothooks/hooks.json— Claude Code's SessionStart registration — and tries to run a hook meant for a different harness.Notes:
hooks/hooks.jsonis untouched.fix(systematic-debugging): Repair find-polluter test-file matchingWhat:
find . -path "./$TEST_PATTERN"so the pattern aligns with the./-prefixed pathsfind .emits../so the prefix is not applied twice.**/collapsed, becausefind's-pathcannot match**/against zero directory levels —src/**/*.test.tswas skippingsrc/top.test.ts.TOTALis 0 instead ofwc -lreturning 1 on empty input.Why:
Notes:
fix(systematic-debugging): Defuse the ultrathink keyword-scanner triggerWhat:
Why:
Notes:
fix(skills): Use resolvable markdown links for reference docsWhat:
@testing-anti-patterns.mdand@testing-skills-with-subagents.mdwith relative markdown links.Why:
@filename.mdform is not a resolvable link, so an agent can treat the filename as literal text or guess at the path instead of opening the file. Both targets sit next to theirSKILL.md.Notes:
@testing-anti-patterns.mdreference uses@prefix that doesnt resolve obra/superpowers#1529) and fix(writing-skills): replace @ reference with markdown link obra/superpowers#631.docs(skills): Remove personal references and clarify skill namesWhat:
receiving-code-reviewwith direct guidance that preserves the behaviour it encoded./Users/jesse/...example paths with generic ones.systematic-debuggingexplicitly in three bootstrap references that said bare "debugging".Why:
Notes:
docs(handoff): Record Batch 1 results and archive the handoff+docs(handoff): Add handoff for the remaining upstream port batchesWhat:
git mvs it todocs/handoffs/_archive/(recorded as a rename, sogit log --followstill works).Why:
Testing
find-polluter.shfunctionally tested in a throwaway toy repo with a stubbednpm: 11/11 checks. The test first reproduces the old failure (old script prints "Found 1 test files", exits 0 claiming clean, never runs the polluter), then verifies the new script counts all three test files including the top-level one, bisects to the polluter, exits 1 on detection, reports 0 honestly when nothing matches, and accepts the pattern with or without a leading./. Test script was scratch-only and is not part of this PR..codex-plugin/plugin.jsonre-parsed as valid JSON with thehookskey present and set to{}.bash -nclean onfind-polluter.sh.shellcheck(not installed on this machine). Upstream ran it on their equivalent change; that gate was not reproduced here.tests/opencode/run-tests.shfails on cleanmain—test-plugin-loading.shrunscp .../liband this repo has nolib/directory. Confirmed by stashing this branch's changes and re-running. Not introduced here; worth fixing separately.Risk and rollout
find-polluter.shnow does real work. Anyone who previously saw "all tests clean" from this script was getting a false negative; they may now get a genuine polluter report or a longer run. That is the fix behaving correctly, not a regression.systematic-debuggingwere silently being pushed into extended thinking. Anyone who had come to rely on that will need to ask for it explicitly.hooks: {}change. It should stop Codex from registering the Claude SessionStart hook; if a Codex user reports the superpowers bootstrap no longer loading, start here.skills/receiving-code-review/SKILL.mdwholesale from upstream. Upstream's copy already contains the same replacement landed in7dea012, so it should overwrite cleanly — but that needs verifying, not assuming.Not in this PR
Batches 2–4 from the review, deliberately deferred. The largest is the brainstorm companion server hardening (upstream obra#1720): this fork ships the 338-line pre-hardening
server.cjswith no authentication and no origin check, which upstream documents as a prompt-injection path into a live session. That is the highest-value remaining item and warrants its own reviewable PR. Full list and reasoning indocs/handoffs/2026-08-05-upstream-ports-batches-2-4.md.🤖 Generated with Claude Code