Skip to content

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

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

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

Conversation

@WODE25500

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, reusing the existing rule-based judge ops (no change to the evidence machinery):

  • 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).

Honest boundaries

  • The scenario pack deliberately does NOT judge whether the agent truly understood the root cause — that is beyond a rule judge (the upstream Superpowers project uses an LLM verifier for skill compliance). It is documented in the module docstring.
  • The change was built and validated offline (16 deterministic unit tests over the judge logic). It was NOT run against a live Claude/Codex harness (the submitting environment has no working authenticated Claude CLI). An opt-in real-harness smoke is documented in the docstring:
    python -m skillopt_sleep.adapters.superpowers --skill systematic-debugging
    A reviewer/CI with a working claude CLI can run it to get real empirical evidence.

Scope

  • 2 files, purely additive (~161 insertions, 0 deletions): skillopt_sleep/adapters/superpowers.py + tests/test_systematic_debugging_scenarios.py.
  • Does not touch the existing verification-before-completion scenarios or the evidence machinery.

Refs #132.

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).
@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.

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.
@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.

_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).
@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.

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