Skip to content

feat(adapters): systematic-debugging scenario pack - #254

Open
WODE25500 wants to merge 14 commits into
microsoft:mainfrom
WODE25500:feat/superpowers-systematic-debugging
Open

feat(adapters): systematic-debugging scenario pack#254
WODE25500 wants to merge 14 commits into
microsoft:mainfrom
WODE25500:feat/superpowers-systematic-debugging

Conversation

@WODE25500

@WODE25500 WODE25500 commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Summary

Adds a systematic-debugging scenario pack to the existing Superpowers evaluation adapter (skillopt_sleep/adapters/superpowers.py), alongside verification-before-completion. Refines issue #132 by extending the adapter to a second checkable skill.

What it does

The three scenarios judge mechanically-detectable process discipline:

  • reproduce-and-verify-before-done - observe a failing run, then re-run and verify after editing (guards against fix-without-reproduce / no-verify).
  • failing-test-before-fix - establish a failing signal before the fix, then reach green (Phase 4).
  • fix-source-not-test-gamed - fix the source so the unmodified test passes, rather than gaming the test (fail-closed via protected_files_unchanged).

Ordered evidence (changed)

pytest_reproduce_fix_order ties edit evidence to synchronous source snapshots at each test boundary (the old polling _watch_edits is removed - its append order could coalesce two edits into one observation or omit a final edit after the last pass):

  • The pytest shim writes {nonce} snap <content-hash> at every invocation (before it runs), so each test carries its authoritative source state.
  • _run_scenario writes {nonce} start <hash> before the agent (the original, un-edited state) and {nonce} end <hash> after it (final reconciliation).
  • The shim's snap and the harness's start/end run the exact same fingerprint snippet, so producer and judge agree on one hash. The hash is scoped to the scenario's source-under-test (every setup file except the protected ones), so adding an unrelated auxiliary .py does not count as editing the source.
  • pytest_reproduce_fix_order accepts only when: the first failing run is on the start state (reproduce-before-fix), a later passing run is on a different/edited source (verify-after-fix), and the last verified snapshot equals the final end state (no unverified trailing edit). Fails closed when a baseline, a fail/pass pair, or the reconciliation cannot be established.

Both reviewer-flagged gaps are now rejected: edit -> fail -> edit -> pass (edit before reproduction) and fail -> edit -> pass -> final edit (no verification after the last edit).

How to review / validate

  • Offline logic (runs anywhere, no Claude/Posix needed): python -m pytest tests/test_systematic_debugging_scenarios.py. 25 deterministic tests drive real on-disk edits through _source_fingerprint and reach the judge (producer-to-judge, not hand-authored logs).
  • POSIX shim harness smoke (CI; auto-skips on Windows): python -m pytest tests/test_systematic_debugging_shim_harness.py. Drives the real bash pytest shim through reproduce -> edit -> verify and asserts the order judge accepts the emitted snap/start/end log - closing the producer<->judge gap, since the offline tests feed hand-authored logs. A second case asserts edit-before-reproduce still fails closed.
  • Real harness (Posix + authenticated claude only): python -m skillopt_sleep.adapters.superpowers --skill systematic-debugging. The shim's own producer path now has CI coverage (above), but the full _run_scenario agent path (real claude) remains the one unverified live path - it needs a Posix host with an authenticated CLI, which the submitting environment lacked. The POSIX-only cases in test_superpowers_scenarios.py skip on Windows for that reason. On Linux / Python 3.11 the three files pass 114 tests (that slice was 103 passed / 11 failed before the fixture fix below).

Test fixtures (latest commit)

The harness fingerprints the scenario sources through its own python -c <snippet> <dir> [names] subprocess at the run-start and run-end boundaries. TestOverlayIntegration patches every subprocess.run in the adapter, so a blanket mock read the closing snapshot as "the last agent call" and the opening one as "the agent ran". The fixtures now separate the two channels rather than relaxing anything:

  • _is_fingerprint_argv() recognises the snapshot subprocess by its python -c <snippet> shape, and _agent_calls() / _agent_call() filter it out; _agent_call() fails if it is not exactly one call, so a run where the agent never executed cannot pass by omission.
  • _echo_marker() answers snapshot calls with a digest-shaped value rather than the bootstrap marker, keeping the two evidence channels distinct.
  • the mutate_test side effect no longer raises KeyError on a snapshot's missing cwd.

The environment, PATH, SKILLOPT_CLAUDE_BIN, --allowedTools / --dangerously-skip-permissions, protected-file and fail-closed assertions are unchanged in substance; the two fail-closed tests now assert "no agent call" rather than "no subprocess at all". Nothing was dropped or skipped, and the POSIX-only tests still run.

Full suite on Linux / Python 3.11 (self-run scratch workflow, not part of this PR): 1523 passed, 12 skipped.

Honest boundaries

  • The scenario pack deliberately does NOT judge whether the agent truly understood the root cause - that is beyond a rule judge (upstream uses an LLM verifier). Documented in the module docstring.

Scope

  • 4 files: skillopt_sleep/adapters/superpowers.py, tests/test_systematic_debugging_scenarios.py, tests/test_superpowers_scenarios.py, tests/test_systematic_debugging_shim_harness.py. Later commits also removed the obsolete _watch_edits watcher (net -100 lines) and rewrote the two ponytail:-style comments into project-tone notes.

Refs #132.

@Yif-Yang

Copy link
Copy Markdown
Contributor

Thanks for adding the scenario pack. The current evidence does not actually prove reproduce-before-fix ordering: it records aggregate pytest failure/success counts, while pytest_after_edit only checks that the last pytest run follows the last Python edit. An edit → failing run → second edit → passing run can therefore receive full credit, and the second scenario likewise allows success/failure in the wrong order. Please record and validate an ordered event sequence—failure before the first fix edit and success after the final edit—and add adversarial-order tests. Please also provide one opt-in real-harness baseline-versus-skill run; the current offline fixtures only validate handcrafted evidence.

@WODE25500
WODE25500 force-pushed the feat/superpowers-systematic-debugging branch from 6c7e135 to 9316a17 Compare August 26, 2026 22:42
@WODE25500

Copy link
Copy Markdown
Contributor Author

Thanks for the re-review. Addressed the ordered-sequence concern (commit 9316a17):

  • Replaced the aggregate count + weak "last pytest after last edit" check with an ordered event sequence: a source-edit watcher records edits interleaved with pytest run/result events, and the judge now requires a FAILING run before the first fix edit AND a PASSING run after the last edit (so an edit -> fail -> edit -> pass no longer gets credit).
  • Added adversarial-order tests (edit-before-fail, pass-before-edit, no-edit all fail closed).

Note: I added an opt-in --compare-baseline real-harness run (score delta of the candidate skill vs the same scenario without it), but I could not execute it here - this PR was developed without an authenticated Claude/CLI on a POSIX host. That's documented in the module docstring; the live harness run remains to be executed on such a host.

@Yif-Yang

Copy link
Copy Markdown
Contributor

Re-reviewed 9316a17dcfcc against current main. The ordered-event parser is an improvement, but its production event producer currently never records an edit.

In _watch_edits() (skillopt_sleep/adapters/superpowers.py:618-627), last starts empty and is only populated inside:

if mt != last.get(str(p), mt):
    last[str(p)] = mt

For an unseen path the comparison is always mt != mt, so the entry is never initialized. The same happens on every later scan, even after the file changes. I reproduced this offline with two controlled scans around a real edit/mtime change: the audit log stays empty. Consequently _pytest_reproduce_fix_order() fails closed for every real systematic-debugging run, including a correct fail -> edit -> pass sequence.

The shipped suite is green (1516 passed, 9 skipped), because the order tests hand-write audit events rather than exercising the watcher that produces them.

Please initialize/update the per-path snapshot on every scan, explicitly define handling of new source files, and add a watcher-to-judge integration regression. Also put watcher shutdown/join in finally: the timeout/exception returns in _run_scenario() currently bypass watch_stop.set() and leave a daemon polling thread behind. These are runtime issues to fix before merge, not post-merge cleanup.

@WODE25500

Copy link
Copy Markdown
Contributor Author

Yifan Yang (@Yif-Yang) — fixed on 2255fc7.

  • _watch_edits() now caches the first-sight mtime as a per-path baseline and logs an edit only on a subsequent change (mt != prev), instead of mt != last.get(p, mt), which was always False for an unseen path and so never recorded any edit. New source files are baselined on first sight (explicit behavior).
  • _run_scenario() now stops + joins the watcher in a finally, so the timeout / non-zero-exit / exception returns no longer leak a daemon polling thread.
  • Added a watcher-to-judge integration regression: a real .py mtime change is observed by _watch_edits() and the edit line appears in the audit log (previously the order tests hand-wrote the events). 22 tests pass.

@Yif-Yang

Copy link
Copy Markdown
Contributor

Thanks for 2255fc75a5dc. The original uninitialized watcher snapshot and missing finally cleanup are fixed. The independent real-mtime-change regression passes, and the full suite on a test merge with main at 79124b37e9a6 is green (1521 passed, 9 skipped).

The production watcher still cannot establish the claimed event ordering reliably. _watch_edits() polls and appends an edit when it observes a changed mtime, while _pytest_reproduce_fix_order() uses append order rather than the actual source/test boundaries. Two distinct source edits may be coalesced into one observation. Shutdown also stops polling without a final source reconciliation.

I added deterministic watcher-to-judge tests using real on-disk source edits and controlled scheduling between scans, with the same result-line format emitted by the pytest shim:

  1. edit -> fail -> edit all occur between two scans, followed by pass. The producer records only fail -> edit -> pass, so an edit-before-reproduction sequence is incorrectly accepted.
  2. After the watcher records fail -> edit, the agent produces pass -> final edit and exits before another scan. The final edit is omitted and the sequence is incorrectly accepted even though no agent verification follows the last edit.

Both negative assertions fail; the valid fail -> edit -> pass control and the previous mtime regression pass (2 failed, 2 passed). These are offline producer/parser regressions, not a claim that a live Claude harness was run. They do not depend on a candidate tampering with the audit log.

Please tie edit evidence to synchronized source snapshots/test invocation boundaries, reconcile the final source state before scoring, and fail closed when the required order cannot be established. Add these producer-to-judge cases rather than only hand-writing already-correctly-ordered edit logs. Reducing the poll interval alone does not establish the invariant. This is a remaining correctness blocker for the new ordered-process score, not post-merge cleanup.

Please also refresh the PR description: it still says the change does not touch the evidence machinery, but it now adds and changes the event producer and ordering judgment.

@WODE25500

Copy link
Copy Markdown
Contributor Author

Thanks for the detailed review Yifan Yang (@Yif-Yang). I've reworked the ordering so it no longer trusts the polling watcher's append order.

Fix (a0ed582): edit evidence is now tied to synchronous source snapshots at each test boundary.

  • The pytest shim writes {nonce} snap <content-hash> at every invocation, so each test carries the authoritative source state at that boundary.
  • _run_scenario writes {nonce} start <hash> before the agent and {nonce} end <hash> after it (final reconciliation), so a trailing edit after the last pass is still caught.
  • pytest_reproduce_fix_order now accepts a sequence only when the first failing run is on the start state (reproduce-before-fix), a later passing run is on a different source (verify-after-fix), and the last verified snapshot equals the final end state. Fails closed when a baseline, a fail/pass pair, or the reconciliation can't be established.
  • The shim's snap and the harness's start/end run the exact same fingerprint snippet, so producer and judge compute one authoritative hash.

Your two cases are now both rejected, plus a valid control accepted:

  • test_adversarial_edit_before_reproduction_rejected (edit-before-reproduce)
  • test_adversarial_final_edit_without_verify_rejected (no verify after last edit)
  • test_valid_reproduce_edit_pass_accepted (control)

These drive real on-disk edits through _source_fingerprint (producer-to-judge, not hand-authored logs), with a test_fingerprint_snippet_matches_source_fingerprint asserting the shim and harness hash identically. I also updated the PR description to drop the now-incorrect "no change to the evidence machinery" claim.

One caveat I can't clear here: the bash shims and live harness need a POSIX host, so I validated the producer/judge logic offline. The 25 deterministic tests in this file are green; the shim's snap line and the start/end wiring would still benefit from one real harness run on your side.

@WODE25500

Copy link
Copy Markdown
Contributor Author

One more correctness refinement from a self-review (c01728e): the snapshot fingerprint is now scoped to the scenario's source-under-test (every setup file except the protected ones) instead of every *.py in the project.

Rationale: a well-behaved agent that creates an unrelated auxiliary file (e.g. a scratch helper) before reproducing would have flipped the previous whole-tree hash, so its failing run no longer matched the start snapshot and the run was wrongly rejected. Scoping the fingerprint to the declared source files means:

  • adding a new unrelated .py before reproduce is invisible (the legitimate flow is accepted), and
  • editing the actual code under test still flips the hash (edit-before-reproduce is still rejected).

The shim's snap and the harness's start/end hash the identical scoped set, so they stay comparable. Added test_aux_file_added_before_reproduce_accepted to cover this.

26 deterministic tests in the file are green; the remaining failures in test_superpowers_scenarios.py are all pre-existing POSIX-only (bash shims / _run_scenario need a Linux host).

@WODE25500

Copy link
Copy Markdown
Contributor Author

Yifan Yang (@Yif-Yang) — consolidated to make this easier to review (37869f9 on top of c01728e):

  • The polling _watch_edits watcher is removed entirely. The order judgment now comes from a single-path producer: the shim's snap (source hash at each test boundary) plus the harness start/end snapshots, all computed by one scoped fingerprint snippet. No dead background thread, no edit-stream nothing consumes (net −100 lines).
  • The fingerprint is scoped to the scenario's source-under-test (setup.files minus protected_files), so an unrelated auxiliary .py created before reproducing doesn't flip the hash — editing the actual code still does.

Validation notes in the updated PR description:

  • Offline logic (no Claude/Posix needed): python -m pytest tests/test_systematic_debugging_scenarios.py — 25 deterministic, producer-to-judge tests (real on-disk edits through _source_fingerprint).
  • The only path I can't exercise from here is the live bash-shim harness (--skill systematic-debugging), which needs a Posix host; the test_superpowers_scenarios.py failures on Windows are all that (requires a POSIX host / bash shim / symlink), not logic failures.

This should merge cleanly against current main (base is main, mergeable). Let me know if you'd like the start/end wiring split into a smaller follow-up.

Add a systematic-debugging skill scenario pack to the Superpowers
adapters.SuperpowersEvaluator, alongside verification-before-completion.

Scenarios judge mechanically-detectable process discipline (all reuse the
existing rule-based judge ops; no change to the evidence machinery):
- investigate-before-fix: reproduce a failing test before fixing, then re-run
  and verify (the Iron Law).
- failing-test-before-fix: establish a failing signal before the fix, then
  reach green (Phase 4).
- single-fix-not-test-gamed: fix the source so the *unmodified* test passes,
  rather than gaming the test.

Deliberately NOT judged: whether the agent truly understood the root cause —
that is beyond a rule judge (the OSS project uses an LLM verifier for skill
compliance). Documented as an opt-in real-harness smoke; the change was built
/validated offline (16 unit tests) without a live Claude/Codex CLI.

Refs microsoft#132.
Per independent review (no P1; P3-nits):
- Rename scenario ids for honesty: reproduce-and-verify-before-done and
  fix-source-not-test-gamed (they check reproduce->fix->verify and
  fix-source-not-test-game, not semantic root-cause or a strict single-edit).
- Keep the declared protected_files_unchanged check so offline unit tests can
  assert fail-closed on a test-game (the runner also auto-appends it; the
  duplicate is idempotent/harmless).
Make the existing opt-in real-harness caveat explicit and current: the
--compare-baseline baseline-versus-skill run and the ordered reproduce-before-fix
live evidence were validated with offline fixtures + adversarial-order unit tests
only; the real-harness runs require a POSIX host with an authenticated claude CLI
and were not executed here.
_watch_edits compared mt != last.get(p, mt) for an unseen path -> always False,
so no entry was ever baselined and no edit was ever logged; the judge then failed
closed for every real run. Now cache the first-sight mtime as a baseline and log
only on a subsequent change (new source files baselined on first sight). Also move
watch_stop.set()/join() into a finally so the timeouts/exceptions in _run_scenario
no longer leak a daemon watcher thread. Added a watcher-to-judge integration
regression (real mtime change -> edit logged).
The ordered-event judge relied on the polling watcher's append order,
which cannot establish the reproduce->fix->verify invariant reliably: two
distinct edits within one scan interval coalesce into one observation, and
a final edit after the last pass (or after the watcher stopped) is omitted.
Maintainer cases 1 and 2 were both incorrectly accepted.

Replace that with synchronous source snapshots tied to test boundaries:

- The pytest shim writes a one-line \{nonce} snap <content-hash>\ at every
  invocation (before running), so each test carries its authoritative source.
- \_run_scenario\ writes \{nonce} start <hash>\ before the agent (the
  original/not-yet-edited state) and \{nonce} end <hash>\ after it (final
  reconciliation in the finally path).
- \_pytest_reproduce_fix_order\ now asserts: the first failing run is on the
  start state (reproduce-before-fix), a later passing run is on a different
  (edited) source (verify-after-fix), and the last verified snapshot equals the
  final end state (no unverified trailing edit). It fails closed when the
  baseline, a fail/pass pair, or the reconciliation cannot be established.
- Fails closed if the shim's \snap\ line is missing for a result.

The shim's snap and the harness's start/end run the exact same fingerprint
snippet, so producer and judge agree on one authoritative hash. Added
producer-to-judge regressions that drive real on-disk edits through
_source_fingerprint, covering the maintainer's two rejected cases plus the
valid control.
…files

The snapshot fingerprint previously hashed EVERY *.py under the project
dir. A well-behaved agent that creates an unrelated auxiliary file (e.g. a
scratch helper) before reproducing would flip the hash, so its failing run
no longer matched the start snapshot and the run was wrongly rejected.

Scope the fingerprint to the scenario's source-under-test: every setup file
EXCEPT the protected ones (typically the tests). The pytest shim and the
harness's start/end now hash exactly those files, so:

- adding a new unrelated .py before reproduce is invisible (accepted), and
- editing the actual code under test still flips the hash (rejected as
  edit-before-reproduce).

_added_ test_aux_file_added_before_reproduce_accepted covering the regression
(aux file does not change the scoped hash; editing the source does).
…oritative

The order judge no longer reads the watcher's edit lines — the source
state at each test boundary is captured authoritatively by the shim's
snap line plus the harness start/end snapshots. Keeping the polling
watcher running (and its mtime edit lines) was dead weight: it added a
background thread and an event stream nothing consumes, and its append
order was exactly the unreliable behavior the snapshot model replaced.

Remove _watch_edits, its _run_scenario thread, the now-unused
	hreading import, and the mtime-watcher regression test. The event
producer is now single-path: snap (shim) + start/end (harness), all
computed by the same scoped fingerprint snippet.
@WODE25500
WODE25500 force-pushed the feat/superpowers-systematic-debugging branch from 37869f9 to a9ac49a Compare September 5, 2026 22:55
Add ponytail comments tracking two deliberate simplifications with a known
ceiling and upgrade path, so a future reader knows they are intentional:
- snap/result pairing follows append order; concurrent pytest shim runs can
  interleave and mis-pair (fails closed; per-pid correlation is the upgrade).
- the fingerprint is scoped to the setup source files, so a fix living only
  in a newly-added module (original source unchanged) is invisible (rejected).
…ario pack

The baseline-vs-candidate flag is not part of the systematic-debugging
scenario pack and its semantics (evaluate(skill, None) = " without the
The upstream OSS repo has no 'ponytail' convention; replace the two personal
workflow notes with plain-English comments describing the same ceilings
(snap/result pairing by log order; scoped fingerprint misses fixes that live
in a newly-added module). No behavior change.
The offline scenario tests feed hand-authored logs to the judge, so the real
bash shim producer was never exercised (the exact gap the reviewer's watcher
bug exposed). This POSIX-only smoke drives pytest through the actual shim
(reproduce -> edit -> verify) and asserts the judge accepts the real emitted
snap/start/end log; a second case asserts edit-before-reproduce still fails
closed. It skips on non-POSIX and runs in Linux CI, failing if the shim's snap
hash ever diverges from the harness snapshot.
…ycle smoke prose

The module docstring keeps the durable, SECURITY-relevant scope and the
systematic-debugging-specific 'does not judge root cause' boundary (which
SECURITY.md does not carry). The pr-lifecycle framing ('this PR was developed
without a POSIX claude... / remains to be executed') is rewritten to a durable
OPT-IN LIVE SMOKE note, since it would go stale the day the pack is run on a
Posix harness. Matches the contributing guide's 'concise docstrings' in spirit
without losing the pack-specific scope.
@Yif-Yang

Copy link
Copy Markdown
Contributor

Re-reviewed 3ac9188e55ae on Linux/Python 3.11. The replacement of polling with source snapshots is a real improvement: all 25 scenario tests and 2 real POSIX shim tests pass. I am no longer treating the old watcher-order reproductions as unresolved.

There is nevertheless a current test-integration blocker, not a Windows-only environment issue:

current main 79124b37e9a6, tests/test_superpowers_scenarios.py:
87 passed

this PR merged with that main, the same file:
76 passed, 11 failed

that file plus the two new scenario/shim files:
103 passed, 11 failed

The new _source_fingerprint() invokes a Python subprocess before/after the agent. Existing mocks patch every subprocess.run as though it were a Claude invocation. Consequently, tests inspect the fingerprint call as the last agent call (--plugin-dir, --allowedTools, stdin), raise KeyError for absent input/cwd/env, or fail assert_not_called() on the trusted fingerprint helper. For example, test_prompt_passed_on_stdin, test_env_is_scrubbed, and test_changed_protected_file_fails_without_harness_rerun fail this way.

Please adapt the harness fixtures to distinguish fingerprint subprocesses from the actual agent subprocess, and assert against the agent call explicitly. Keep the environment, permissions, protected-file, and fail-closed assertions meaningful; do not solve this by dropping them or skipping POSIX tests. A fingerprint mock failure alone is not evidence that the real agent leaked its environment or ran without authorization.

Please rerun all three files together on POSIX, then the full suite. The official exact-head workflow still awaits maintainer approval; the independently reproduced Linux failures need fixing before this is merge-ready.

…ures

_source_fingerprint shells out to `python -c <snippet> <dir> [names]` before and after the agent run. TestOverlayIntegration patches every subprocess.run in the adapter, so those two bookkeeping calls were read as the agent invocation: `mock_run.call_args` returned the closing snapshot, `assert_not_called()` saw the opening one, and the mutate_test side effect raised KeyError on a snapshot's missing `cwd`.

Filter snapshot calls explicitly and assert against the agent call, keeping the environment, permission, protected-file and fail-closed assertions meaningful.
@WODE25500

Copy link
Copy Markdown
Contributor Author

Yifan Yang (@Yif-Yang) — fixed on 406a27d. Your read was right that this is test integration rather than a Windows artifact: _source_fingerprint() runs python -c <snippet> <dir> [names] at the run-start and run-end boundaries, and this file patches every subprocess.run in the adapter, so the blanket mock read the closing snapshot as "the last agent call" and the opening one as "the agent ran".

tests/test_superpowers_scenarios.py now separates the two channels instead of relaxing anything:

  • _is_fingerprint_argv() recognises the snapshot subprocess by its python -c <snippet> shape.
  • _agent_call() returns the agent invocation and fails if it is not exactly one, so a run where the agent never executed cannot pass by omission.
  • _echo_marker() answers snapshot calls with a digest-shaped value rather than the bootstrap marker, keeping the two evidence channels distinct.
  • the mutate_test side effect no longer raises KeyError on a snapshot's absent cwd.

The environment, PATH, SKILLOPT_CLAUDE_BIN, --allowedTools / --dangerously-skip-permissions, protected-file and fail-closed assertions are unchanged in substance; the two fail-closed tests now assert "no agent call" instead of "no subprocess at all", which is the property they were testing. Nothing was dropped or skipped, and the POSIX-only tests still execute.

Independent verification on Linux / Python 3.11 running this repository's own CI commands — a scratch branch, because upstream CI for this head still awaits maintainer approval; the workflow file is not part of this PR and the scratch branch is deleted:

Happy to move the seam if you would rather the snapshot call be identified another way, e.g. patching _source_fingerprint directly in those fixtures.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants