Skip to content

Study Command - #277

Open
FrankRay78 wants to merge 2 commits into
mainfrom
feature/260-study-command
Open

Study Command#277
FrankRay78 wants to merge 2 commits into
mainfrom
feature/260-study-command

Conversation

@FrankRay78

Copy link
Copy Markdown
Owner

Why

/build and /verify now carry a piece of work from a GitHub issue to a reviewed, test-green branch, but neither leaves any record of where the work went sideways on the way. A reviewer finding gets fixed and disappears into a commit; an ambiguous criterion gets interpreted and never mentioned again. Individually invisible, collectively they are the only evidence of whether the harness itself needs work.

/study records each genuine surprise as one classified row, so the accumulated files eventually answer a question no single PR can: which part of the harness keeps costing us, and therefore what to fix.

What changes

Adds a /study command and the docs/study/ folder it writes into. No existing command is altered.

Rows are classified to exactly one of Execution (the doing went wrong), Plan-spec (the criteria were wrong), Codebase (the existing code was not what anyone assumed), or Environment (the surprise came from outside the repo). The levels are not severities — they say where the fix belongs, which is what makes a recurring level actionable.

Two guards keep the folder worth reading: a clean run writes nothing at all (there is no "nothing notable" row, so a file's presence means the work taught us something), and every row must be grounded in evidence the run can point at. Asked what surprised it, a model will readily invent plausible surprises; a folder of confabulation would be worse than no folder.

Non-obvious things a reviewer should know

Read the issue's ## Confirmed decisions alongside its acceptance criteria. Ten bullets there override several defaults — the three-column self-grounding table (no Evidence column), the flat append-only shape (no per-pass sections or dates), the bare-number filename, refusing to run from main, and the verdict line. A review against the AC checklist alone would flag correct behaviour as wrong.

There are no tests, deliberately. Constitution §I's carve-out for configuration and tooling applies: the artefact is a prompt file, no code is added, and nothing under src/ is touched. The RED evidence is the real gate — before the change, .claude/commands/study.md did not exist, docs/study/ did not exist, and nothing in the repo defined /study; after it, the harness parsed the file and registered the command with its description. The issue's confirmed decisions record this explicitly and direct that no bespoke markdown test be written.

Three **Scenario:** labels carry zero // SCENARIO: markers, and that is deliberate rather than forgotten. The traceability gate reads specs/*/spec.md, which does not exist here and which /build forbids creating, so the gate is a verified no-op. The only way to host a marker would be an xUnit test asserting over prose in .claude/commands/ — the hand-rolled stand-in §I explicitly bans, and unable to judge criteria like "only genuinely new findings are added" in any case. The scenarios are instead satisfied by named steps in the command.

This invariant is verified once, by hand, not gated. §I also requires making a check repeatable in CI where the tool can run there. No tool can decide whether a prompt is correct, so that clause is vacuous here — worth knowing rather than worth pretending otherwise.

The second commit is larger than the first, and reshaped the command. Four clean-context reviewers audited the prompt as a program — its steps as control flow, its failure clauses as error handling — and three independently found the same missing unwind path. The substantive fixes: rows=0 could not distinguish "nothing was surprising" from "I could not look" (now a three-state had/absent/errored classification per source, with a FAILED reason=no evidence gathered stop); a rejected commit left a written, staged file while the report asserted the repository was unchanged, dirtying the tree and blocking /verify — the one thing the command promises never to do. That path is real in this repo, not hypothetical: no-skipped-tests.sh is a PreToolUse hook on git commit that fails closed.

Two review findings were rejected on purpose. Making README.md the normative source for the four levels with a cross-reference from the command would contradict feedback_prompts_locality_over_dry — the executor should not need a second file open. And the third "never prompts" statement was kept because its position in the ## User Input block mirrors raise-pr.md verbatim and is load-bearing exactly where an empty argument is discussed.

One line lands outside docs/study/. docs/agentic-workflow-NetPace.md gains a single Related bullet, because two reviewers independently noted the README linked outward to the workflow map while the map never mentioned the folder. That documents this branch's own addition rather than folding in a second mission.

How to verify

  • Walk the 15 acceptance criteria and the 10 confirmed decisions in Add /study — record what surprised a build, so the harness can be improved from evidence #260 against .claude/commands/study.md — the checklist alone is not the whole contract.
  • Check the failure paths specifically: refusing main, refusing a detached HEAD, an un-inferable issue number, and the step-6 rollback when a commit is rejected.
  • Confirm branch inference cannot misfile: feature/net10-upgrade must not yield 10, and a bare NNN-<slug> spec-kit branch must yield no candidate. This mirrors /raise-pr's existing rule.
  • Confirm the command still never prompts, never pushes, opens or merges a PR, never runs tests or formatting, and never edits CLAUDE.md, .claude/memory/ or hooks.
  • Read docs/study/README.md as a future analyst would, and check the four levels are distinguishable enough to classify against.

Related

Closes #260

🤖 Generated with Claude Code

https://claude.ai/code/session_01YaRuiUQHdPMeGugFuvGfeK

FrankRay78 and others added 2 commits September 7, 2026 16:09
Add a /study command that records each genuine surprise from a piece of
work as one classified row in docs/study/<issue>.md, so the accumulated
files answer a question no single PR can: which part of the harness keeps
costing us, and therefore what to fix.

Two guards keep the folder all-signal: a clean run writes nothing (there
is no "nothing notable" row, so a file's presence means the work taught
us something), and every row must be grounded in evidence the run can
point at — a commit, a review comment, a failing test, a CI run.

Rows are classified to exactly one of Execution, Plan-spec, Codebase or
Environment; the levels say where the fix belongs, which is what makes a
recurring level actionable. There is deliberately no Goal level: whether
an issue should have existed is settled at drafting time, not downstream,
so the level would sit empty and dilute the tally.

/study never prompts, refuses to run from main, changes no source, test
or configuration file, runs no tests, applies no formatting, and never
pushes, opens a PR or merges. A finding that looks mechanically
enforceable is flagged for /capture-learnings rather than acted on here.

docs/study/README.md explains the four levels and how to read the
accumulated files back. No existing command is changed.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01YaRuiUQHdPMeGugFuvGfeK
Four clean-context reviewers audited the new command prompt as a program:
its steps are the control flow and its specified failure behaviour is the
error handling. Three independently found the same missing unwind path.

Blockers fixed:

- rows=0 could not distinguish "nothing was surprising" from "I could not
  look". Step 3 now classifies every evidence source as had / absent /
  errored, never folding errored into absent, and STOPs with
  FAILED reason=no evidence gathered when no source was had.
- Step 6 had no rollback, so a rejected commit left a written, staged file
  while the report asserted the repository was unchanged — dirtying the
  tree and blocking /verify, the one thing the command promises never to
  do. The path is real in this repo: no-skipped-tests.sh is a PreToolUse
  hook on git commit that fails closed.
- Step 3's evidence-source wording pointed at the artefact rather than the
  chat report, against the issue's confirmed decision that the file
  carries no evidence-sources line. Step 5 now states the file contains
  only the H1 and the table.
- The re-run "everything already recorded" path fell through into the
  commit step and would have reported FAILED where AC 7 requires rows=0.
- The description of /capture-learnings dropped its top-ranked
  destination; its actual order is fix the rule that misfired, then
  enforce deterministically, then a memory entry.

Also: branch inference now mirrors /raise-pr's guards, so feature/net10-
upgrade cannot yield 10 and a spec-kit NNN- branch cannot file against a
sequence number; a detached HEAD is refused rather than committed onto;
step 1 takes a baseline so "left pre-existing changes untouched" is
measured rather than asserted; the title fetch moved into the new-file
branch so a clean run is no longer turned into FAILED by an unrelated gh
failure; and /study now refuses to author a replacement README rather
than confabulating the document that defines its own classification.

Docs: the workflow map now links the records folder, and the README drops
an unverifiable claim about an unnamed repository and an example H1 that
was coupled to a live GitHub issue title.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01YaRuiUQHdPMeGugFuvGfeK
@FrankRay78

Copy link
Copy Markdown
Owner Author

@claude Review this pull request. Analyse the code changes and provide feedback covering:

  • Bugs or correctness issues
  • Security concerns (including any flagged by static analysis)
  • Adherence to the project conventions in CLAUDE.md
  • Test coverage — do the tests adequately cover the new behaviour?
  • Any spec/test-plan mismatches

End the review with a recommendation whether to merge the PR, and if not, what you suggest needs addressing first.

@claude

claude Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Claude finished @FrankRay78's task in 3m 17s —— View job


Review complete ✅

  • Gather context (diff, changed files, PR body confirmed decisions)
  • Read .claude/commands/study.md
  • Read docs/study/README.md
  • Read docs/agentic-workflow-NetPace.md diff
  • Check adjacent commands (raise-pr.md, build.md, verify.md, capture-learnings.md) for consistency
  • Verify constitution/CLAUDE.md conventions (TDD carve-out, locality-over-DRY, etc.)
  • Post full review

(Note: gh and network-fetch tools were unavailable in this run, so issue #260's body/confirmed-decisions couldn't be re-fetched independently — the review below relies on the PR description's summary of those decisions plus the diff itself.)


Bugs / correctness issues

Minor — the pre-existing-file guard in step 6 may not catch an untracked leftover. .claude/commands/study.md step 6 says:

If docs/study/<N>.md appears in the step-1 baseline as already modified or staged, STOP and report instead...

This correctly protects against sweeping in someone else's uncommitted edit to an existing tracked file. But if a prior /study run wrote a brand-new docs/study/<N>.md and then hit the documented worst case in the same step ("If you genuinely cannot restore it, say so and name the paths"), the leftover would sit in the working tree as untracked (?? in git status --porcelain), not "modified or staged". A later /study run would then treat it as an "Existing file" in step 5 and silently append fresh rows to — and commit — content it never verified originated from a legitimate run. Worth tightening the guard's wording to also cover an untracked pre-existing file at that path, or having step 6 stop if the path is untracked-but-non-empty in the baseline. This is a narrow, low-probability edge case (it requires a prior run to have already failed its own rollback), not a blocker.

No other correctness issues found — the step-1 detached-HEAD/main guard, the branch-inference rule (correctly excluding feature/net10-upgrade10 and bare NNN-slug spec-kit branches), the had/absent/errored evidence classification, and the commit-rollback path in step 6 are all internally consistent and mirror the equivalent logic already established in raise-pr.md/build.md.

Security concerns

None. This is a prompt-only change (.claude/commands/study.md + two docs files) — no source, tooling, or CI code is added. $ARGUMENTS is interpolated into the prompt the same way build.md/raise-pr.md already do, and the command never executes user-supplied text as a shell command itself (all git/gh invocations use normalized, validated values). No static-analysis-relevant surface is touched.

CLAUDE.md / constitution adherence

  • TDD carve-out (Constitution §I) is correctly applied: this is a tooling artefact, not production code, and the PR body documents the real RED (file/command didn't exist) → GREEN (harness parses it) evidence rather than a hand-rolled test standing in for the gate. Consistent with the carve-out's own stated rationale (dotnet format rewrites all 79 .cs files: insert_final_newline disagrees with every stored file #249).
  • No skipped tests / no bespoke markdown test — correctly avoided per the constitution's explicit ban on hand-rolled stand-ins for a tool that already performs the check.
  • Locality over DRY (feedback_prompts_locality_over_dry) — the four levels are intentionally duplicated between study.md and docs/study/README.md rather than cross-referenced, matching prior project guidance that the executor shouldn't need a second file open. Confirmed as a deliberate, previously-litigated choice per the PR body, not an oversight.
  • No spec/ references, no forward references to unimplemented features, markdown soft-wrapped — all confirmed clean in the diff.
  • Git workflow conventionsRefs #<N> (never a closing keyword) used consistently in the command's own instructions for the commit it makes.
  • Final-report verdict shape (STUDIED issue=<N> rows=<n> / FAILED reason=<short reason>) is consistent with build.md's READY/FAILED and verify.md's VERIFIED/FAILED conventions.

Test coverage

None added, and per the PR body and Constitution §I's carve-out, none is expected for a prompt-only artefact — agreed this is the right call, not a gap. The traceability point (three **Scenario:** labels with no // SCENARIO: test markers) is explained as a verified no-op of the traceability gate (which only reads specs/*/spec.md, absent here by design) — reasonable given /build's own established pattern of deliberately not creating a specs/ folder.

Spec/test-plan mismatches

None — there is no spec-kit specs/ folder for this branch (correctly, by design, matching /build's convention for issue-driven work). The only "spec" is issue #260 itself, and the PR body's walkthrough of its confirmed decisions (three-column table, no Evidence column, flat append-only shape, bare-number filename, refuse-main, verdict line) all check out against what's actually in .claude/commands/study.md and docs/study/README.md.

Recommendation

Merge. The command is internally consistent, follows the established conventions of /build//verify//raise-pr, and the constitution's TDD carve-out is correctly and explicitly justified rather than assumed. The one item raised above (untracked-leftover edge case in the step-6 guard) is minor, narrow, and non-blocking — worth a follow-up tightening but not worth holding this PR for.
· Branch

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.

Add /study — record what surprised a build, so the harness can be improved from evidence

1 participant