Skip to content

Classify every outcome cli.py records as a success or an error - #69

Merged
dmccoystephenson merged 3 commits into
mainfrom
fix/classify-every-recorded-outcome
Jul 29, 2026
Merged

Classify every outcome cli.py records as a success or an error#69
dmccoystephenson merged 3 commits into
mainfrom
fix/classify-every-recorded-outcome

Conversation

@dmccoystephenson

@dmccoystephenson dmccoystephenson commented Jul 29, 2026

Copy link
Copy Markdown
Member

Summary

  • state.SUCCESS_OUTCOMES was missing three of the seven outcome values
    cli.py actually records.
    It listed tend/created/report;
    implement and file-issue (cmd_align records mode.value verbatim
    on success, cli.py:473) and created_incomplete (cli.py:633) were in
    neither that set nor repo_stats()' outcome = 'error' arm, so they
    counted as neither a success nor an error.
  • User-visible effect: a repo whose recorded runs were all successful
    align --implement runs came back from repo_stats() with
    successes: 0 and last_success: NULL. The dashboard's garden view
    reads both straight off that — healthOf(row) hits
    if (!row.last_success) return "struggling" — so it drew a brown,
    drooping plant with a 0 in the Tends column for a repo that had never
    failed. docs/DASHBOARD.md's mapping table documents that stem height
    comes from successes and droop from last_success, so this was the
    documented behaviour quietly not holding.
  • Fix: add all three to SUCCESS_OUTCOMES. created_incomplete is a
    success on purpose — the <slug>-dev-loop skill was created and
    usable (_run_tend_dispatch proceeds straight to the real tend dispatch
    after it, cli.py:657-669); what's incomplete is create-dev-loop's own
    Step 6 GitHub tracker repo, which says nothing about the target repo's
    health.
  • Stated the vocabulary once: new ERROR_OUTCOME and KNOWN_OUTCOMES
    in state.py. Nothing reads KNOWN_OUTCOMES at runtime; it exists so
    the tests below can assert the classification is total. repo_stats()'
    SQL now binds ERROR_OUTCOME rather than repeating the 'error'
    literal.
  • Guarded the gap that let this happen. SUCCESS_OUTCOMES' original
    comment said it was a set rather than outcome != "error" "so a future
    outcome has to be classified deliberately" — that mechanism only
    surfaces a new outcome if something actually checks. Now two things do:
    • tests/test_state.py records one run per KNOWN_OUTCOMES value and
      asserts successes + errors == runs, plus explicit regression cases
      for a successful --implement/--file-issue run and a
      created_incomplete bootstrap.
    • tests/test_cli.py asserts every Mode value that gets recorded
      verbatim is in KNOWN_OUTCOMES (skipping CREATE_DEV_LOOP, the one
      Mode that records created/created_incomplete instead of its own
      value — both asserted separately).
  • docs/ARCHITECTURE.md module tree brought back in line with
    CLAUDE.md's "lists every file in gardener/ with an accurate one-line
    description" contract: __init__.py and __main__.py were missing
    entirely, and cli.py's one-liner listed four of its eight subcommands
    (cli.py's own module docstring already had all eight).
  • docs/TESTING.md's per-module coverage description updated to match the
    new assertions.

Test plan

  • python3 -m compileall gardener — clean
  • PYTHONPATH=. python3 -m unittest discover -s tests -v496
    passed
    (491 before, 5 new)
  • The new tests were confirmed to be real regression guards, not
    tautologies:
    re-run with SUCCESS_OUTCOMES monkeypatched back to
    the pre-fix {"tend", "created", "report"}, all 5 new assertions
    fail ('file-issue' not found in frozenset(...),
    'created_incomplete' not found in ..., and the repo_stats
    success-count assertions). Restored afterward; the throwaway script
    lived in /tmp, nothing added to the repo.
  • No manual dispatch verification needed for this change.
    CLAUDE.md's "Testing changes" requires a real claude dispatch
    only for changes touching dispatch.py, dev_loop.py, or a prompt
    template/preamble. This PR touches state.py, two test files, and
    two docs — no dispatch-layer behaviour, no claude invocation
    shape, and no change to what any mode is permitted to do. The
    automated gate above is the full gate here.

Note on existing data

This changes how already-recorded history is aggregated, not just future
runs — which is the point: previously-uncounted implement/file-issue/
created_incomplete rows will now show up in the dashboard's Tends column
and can move a repo's health from "struggling" to a real value. No schema
change, no migration; repo_stats() recomputes from the same rows.

Closes #67
Closes #68


This PR description was drafted during a Gardener session (Stephenson-Software/gardener).

`state.SUCCESS_OUTCOMES` listed only `tend`/`created`/`report`, but
`cli.py` records four more values: `implement` and `file-issue` (cmd_align
records `mode.value` verbatim on success), `created_incomplete`, and
`error`. The first three fell into neither `SUCCESS_OUTCOMES` nor
`repo_stats()`' `outcome = 'error'` arm, so they were counted as neither —
leaving a repo whose runs all succeeded with `successes: 0` and a NULL
`last_success`, which the dashboard's `healthOf` draws as a struggling
plant with zero tends.

Add all three to `SUCCESS_OUTCOMES`, introduce `ERROR_OUTCOME`/
`KNOWN_OUTCOMES` so the vocabulary is stated in one place, and guard the
gap in tests: `test_state.py` asserts every `KNOWN_OUTCOMES` value lands
in exactly one of the two buckets, and `test_cli.py` asserts every `Mode`
value a run records verbatim is a known outcome. All five new assertions
were confirmed to fail against the pre-fix set.

Also bring `docs/ARCHITECTURE.md`'s module tree back in line with
CLAUDE.md's "lists every file in gardener/" contract: add `__init__.py`
and `__main__.py`, and list all eight of cli.py's subcommands instead of
four.

Closes #67
Closes #68

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@dmccoystephenson

Copy link
Copy Markdown
Member Author

Self-review

No reviewer is configured for this repo (no CODEOWNERS, no bot reviewer),
so this is the inline self-review the dev-loop skill's Phase 4 calls for,
performed in-session. Posted as a comment rather than a formal Review
object with anchored inline comments: gh pr review and
gh api .../pulls/<n>/reviews are both deliberately absent from this
dispatch's allow-list (see dispatch.py's module docstring point 9), so
each inline finding is folded in below as a path:line — finding note.

Overall: the fix is correct and the change is small, but I found two
things worth addressing before merge — one real duplication the PR
arguably makes worse, and one doc line this change renders imprecise. Both
are addressed in a follow-up commit on this branch.

Checked and clean

  • bypassPermissions reachability — re-verified, since CLAUDE.md
    requires a fresh check whenever the dispatch layer could be affected.
    This PR does not touch dispatch.py, MODE_SPECS, tend_mode_spec(),
    or _build_invocation; git diff --stat is state.py + two test files
    • two docs only. ALLOWED_PERMISSION_MODES still excludes
      bypassPermissions and _build_invocation's runtime raise is intact
      and untouched.
  • Stdlib-only — no new imports of any kind, let alone a pip one.
  • No claude flag added outside MODE_SPECS — n/a, no dispatch change.
  • SQL parameter ordering (gardener/state.py:229) — the query now has
    three bind sites in order: {placeholders} for successes, the new ?
    for errors, {placeholders} for last_success. The tuple is
    (*successes, ERROR_OUTCOME, *successes), which matches. Verified by
    the tests actually passing against a real sqlite3 db rather than by
    reading alone — a mis-ordering here would silently count errors as
    whichever outcome sorted first.
  • created_incomplete classified as a success — traced
    cli.py:629-669 to confirm this is right and not just convenient:
    step6_gap is only computed when create_result.ok, and the code
    proceeds to the real tend dispatch afterward. It is a completed
    bootstrap with a missing external tracker repo, not a failed one.
  • Tests are real guards, not tautologies — all 5 new assertions were
    re-run against SUCCESS_OUTCOMES monkeypatched back to the pre-fix set
    and all 5 fail. Without that check, test_every_known_outcome_... in
    particular would be near-vacuous, since it derives its expectations from
    the same constant it's testing.
  • No test invokes a real claude/gh/gittest_state.py's new
    cases use the existing tmp-dir sqlite3 fixture; test_cli.py's new
    class touches only two module-level constants.

Findings

1. gardener/state.py:71 and tests/test_cli.py:555 — the
created/created_incomplete literals are now spelled in three places,
and the new drift guard can't see two of them.

cli.py:633 constructs these strings; state.py now repeats them in
SUCCESS_OUTCOMES; test_cli.py's
test_the_create_dev_loop_bootstrap_outcomes_are_known repeats them a
third time. The Mode-derived half of the guard is genuinely coupled to
its source — add a Mode and it fails. This half is not: rename
created_incomplete in cli.py and all three copies drift apart in
silence, which is the exact failure mode issue #67 is about. Since the
whole point of this PR is "state the vocabulary once", leaving the one
pair of literals that isn't derived from an enum as free-floating
strings undercuts it. Fix: name them in state.py and have cli.py:633
use those constants.

2. docs/DASHBOARD.md:33 — "All-time successful tends (successes)" is
now imprecise in a way this PR causes.

Before this change successes already counted report and created
runs, so the wording was loose; after it, it also counts implement,
file-issue, and created_incomplete, and a repo touched only by
gardener align will now grow a visible stem. CLAUDE.md names this
table a documentation source of truth precisely because "nothing in it is
invented", so it should say successful dispatches, not tends. Not
changing the in-page column header (Tends) — that's a UI change outside
this PR's scope, and the doc being precise about a loosely-labelled column
is better than both being wrong.

Non-findings considered and dismissed

  • test_every_known_outcome_counts_as_a_success_or_an_error builds
    timestamps as 2026-07-{i+1:02d}, which would produce a nonsense day
    number if KNOWN_OUTCOMES ever exceeded 31 entries. Not worth guarding:
    the column is TEXT and every comparison in repo_stats is
    lexicographic, so the test would still be correct, just ugly.
  • docs/ARCHITECTURE.md's tree indents cli.py one column shy of every
    other entry. Pre-existing, left alone — reflowing the whole block would
    bury the two real additions in whitespace noise.

dmccoystephenson and others added 2 commits July 29, 2026 03:41
…rals

Addresses both findings from this PR's self-review.

The `created`/`created_incomplete` strings were spelled in three places —
cli.py's record site, state.py's SUCCESS_OUTCOMES, and the new test — and
unlike every other non-error outcome they aren't derived from a Mode, so
the drift guard this PR adds couldn't see them. Name them in state.py and
have `_run_tend_dispatch` record through those constants. test_state.py
keeps the bare literal deliberately: that string is already in rows of
every existing gardener.sqlite3 and must keep classifying correctly even
if the constant is renamed.

Also make docs/DASHBOARD.md's plant/data table say what `successes` now
actually counts — successful dispatches, not just tends, so an align run
or a dev-loop bootstrap grows the plant too.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The two bootstrap tests exercised the created/created_incomplete branches
but only asserted on stderr and notification counts, so nothing checked
which string actually reached the db. That left TestRecordedOutcomeVocabulary
asserting a constant against itself for this one pair. Assert the recorded
outcome directly, as both the bare literal (what old sqlite3 rows hold) and
the state constant (what cli.py now records through).

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@dmccoystephenson
dmccoystephenson merged commit 4a5bc42 into main Jul 29, 2026
2 checks passed
@dmccoystephenson
dmccoystephenson deleted the fix/classify-every-recorded-outcome branch July 29, 2026 03:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant