From 403bd8a6a2db50e117037d32df46e9bf3cf0eac6 Mon Sep 17 00:00:00 2001 From: prode Date: Sun, 18 Oct 2026 02:31:33 -0300 Subject: [PATCH 1/2] fix(hooks): say unpushed work once at session start, not at the end of every turn On Stop, additionalContext continues the conversation, so the unpushed-work line read as an order and re-fired every turn spent waiting on reviews, on the user, or on a branch nobody wanted pushed yet. It also asked for a push ahead of the reviews delivery.md puts first. It now appears at SessionStart, beside drift, and points at the delivery sequence instead of a bare push. Plan: plans/undelivered-at-session-start.md --- README.md | 13 +++--- docs/wiki/changelog.md | 2 + docs/wiki/pages/hooks.md | 16 ++++--- internal/cli/hooks_agent.go | 65 ++++++++++++++------------- internal/cli/hooks_drift_test.go | 44 +++++++++++++++--- internal/hooks/harness.go | 4 +- plans/undelivered-at-session-start.md | 37 +++++++++++++++ 7 files changed, 132 insertions(+), 49 deletions(-) create mode 100644 plans/undelivered-at-session-start.md diff --git a/README.md b/README.md index fb47796..579ceb0 100644 --- a/README.md +++ b/README.md @@ -49,18 +49,19 @@ wires both places a check can run without anybody remembering it: | | Where | What it does | |---|---|---| | **git** | `.git/hooks` | `pre-commit` runs `scc validate`, `commit-msg` reads the message being written, `pre-push` runs `scc validate --pr --checks`. These **refuse** — a commit is a decision with a natural place to stand in front of. | -| **harness** | `.claude/settings.json` | `SessionStart` says once that this workspace has recorded no commands and reports methodology drift; `UserPromptSubmit` says how to read an artifact the prompt named, and nothing otherwise; `Stop` hands back `scc validate` findings and undelivered work at the end of a turn — the moment they are cheapest to fix. | +| **harness** | `.claude/settings.json` | `SessionStart` says once that this workspace has recorded no commands and reports methodology drift; `UserPromptSubmit` says how to read an artifact the prompt named, and nothing otherwise; `Stop` hands back `scc validate` findings at the end of a turn — the moment they are cheapest to fix. Unpushed work is said once, at `SessionStart`. | -The harness hooks **never refuse**, and they **never push or open a PR** — the Stop -hook says the branch has commits that never left the machine, and the agent pushes in -its next turn, in the open, where you can still stop it. Publishing on an event nobody +The harness hooks **never refuse**, and they **never push or open a PR** — the +SessionStart hook says the branch has commits that never left the machine, and the +agent pushes when its delivery sequence says to, in the open, where you can still stop it. Publishing on an event nobody is watching is not a thing scc does on your behalf. **On `Stop`, saying anything is a continuation**, not a passive report: the harness keeps the conversation going so the agent can act on it. So the Stop hook honours `stop_hook_active` and carries only what the next turn can *resolve* — a finding clears -when it is fixed, unpushed work clears when it is pushed. v0.24.0 got this wrong and -looped; v0.24.1 is the fix. +when it is fixed. v0.24.0 got this wrong and looped; v0.24.1 is the fix. Unpushed work is +no longer on Stop: a push is not always the next move, and the line held every turn +spent waiting on reviews or on you until it was obeyed. **Drift is the half no validator can reach**, because none of them reads source: code changed on this branch with no box ticked, a `TODO` left behind, commits sitting on the diff --git a/docs/wiki/changelog.md b/docs/wiki/changelog.md index 881828e..572f79d 100644 --- a/docs/wiki/changelog.md +++ b/docs/wiki/changelog.md @@ -7,6 +7,8 @@ thing it describes moved. A page named here that no longer exists is reported, so a rename is recorded as a change rather than left pointing at the old slug. +- 2026-10-18 — [[hooks]]: unpushed work moved from `Stop` to `SessionStart`, and the + line points at the delivery sequence rather than at a bare push. - 2026-10-18 — [[hooks]]: `SCC_SKIP_CHECKS`, the narrow escape past a delivery-gate command this machine cannot run, beside `SCC_SKIP_HOOKS`. - 2026-09-19 — `CLAUDE.md` went back to being the entry file the binary ships, so diff --git a/docs/wiki/pages/hooks.md b/docs/wiki/pages/hooks.md index 578bd2f..58facff 100644 --- a/docs/wiki/pages/hooks.md +++ b/docs/wiki/pages/hooks.md @@ -66,8 +66,11 @@ cap: measured in a real session, the agent answered `.` to itself nine times. Two rules now hold it: **`stop_hook_active` is honoured unconditionally**, and **Stop may only carry what the next turn can resolve.** A finding clears when fixed; -unpushed work clears when pushed; a fact about the branch clears for nobody, which -is why drift moved to `SessionStart`. +a fact about the branch clears for nobody, which is why drift moved to `SessionStart`. +Unpushed work followed it there: a push clears it, but a push is not always the right +next move — reviews still running, a question to the user still open — and at Stop the +line held every one of those turns open until it was obeyed, a push ahead of the +reviews [[delivery-gate]] puts first. That second rule had two more violations of the same shape. The stage ran every validator, `attribution` included — whose subject is commits already made, so the @@ -111,10 +114,11 @@ it, which a test pins. ### They never publish The end of a turn is a plausible moment to `git push`, and doing it would send work -on an event nobody is watching. So the Stop stage *says* the branch has commits that -never left the machine, and the agent pushes in its next turn, in the open, where a -person can still stop it. The question is answered from refs already in the -repository — no fetch, no `gh` — because this runs at the end of every turn. +on an event nobody is watching. So `SessionStart` *says* the branch has commits that +never left the machine, and the agent pushes when the delivery sequence says to, in +the open, where a person can still stop it. The question is answered from refs +already in the repository — no fetch, no `gh` — so the session starts without waiting +on the network. ## No `SubagentStart` diff --git a/internal/cli/hooks_agent.go b/internal/cli/hooks_agent.go index b55c45f..655b2f7 100644 --- a/internal/cli/hooks_agent.go +++ b/internal/cli/hooks_agent.go @@ -33,18 +33,18 @@ import ( // // - `stop_hook_active` is honoured unconditionally. Once the harness says it is // already continuing because of this hook, the hook has been heard. -// - Stop may only carry what the next turn can *resolve*. A finding clears when -// it is fixed; unpushed work clears when it is pushed. A fact about the -// branch clears for nobody, which is why drift is reported at SessionStart. +// - Stop may only carry what the next turn can *resolve*, and should. A +// finding clears when it is fixed. A fact about the branch clears for nobody, +// and unpushed work clears only by a push the agent may rightly not be ready +// for, which is why both are reported at SessionStart. // // The git hooks are where refusal belongs, because a commit is a decision with a // natural place to stand in front of. // // **They never publish.** The end of a turn is a plausible moment to `git push`, -// and scc does not: that sends work on an event nobody is watching. What the Stop -// stage does instead is say the branch has commits that never left the machine, -// so the agent pushes in its next turn — in the open, where a person can still -// stop it. +// and scc does not: that sends work on an event nobody is watching. SessionStart +// says the branch has commits that never left the machine, and the agent pushes +// when delivery.md says to — in the open, where a person can still stop it. // // **And they cost nothing a turn cannot afford.** No network: the pull-request // check and the test suite are both off here, and undelivered work is answered @@ -191,6 +191,14 @@ func sessionStartContext(root string) []string { // until the harness cuts it off. Said once, at the start, where it is // information rather than an instruction the agent cannot carry out. out = append(out, driftLines(root)...) + // Unpushed work is here for the same reason. At Stop it read as an order, and + // the next turn obeyed it: a push while reviews were still running, while the + // user had not answered, on a branch nobody wanted pushed yet — and while the + // agent waited, the line came back at the end of every turn. Whether the work + // is finished is the agent's call and delivery.md's sequence, not the tree's. + if line := undeliveredLine(root); line != "" { + out = append(out, line) + } cfg, err := gate.Load(root) if err != nil || cfg.Any() { @@ -248,15 +256,18 @@ func unattendedLines() []string { } } -// stopContext is what the end of a turn has to say: what the validators found, -// and whether finished work is still sitting on this machine. +// stopContext is what the end of a turn has to say: what the validators found. // // **Only things the agent can resolve now.** Anything here continues the // conversation, so whatever it says has to be something the next turn can act on // and thereby make go away — otherwise the same line comes back on the next stop, // and the next, until the harness overrides the hook at its consecutive-block -// cap. A finding clears when it is fixed; unpushed work clears when it is pushed. -// Both terminate. +// cap. A finding clears when it is fixed, so it terminates. +// +// Unpushed work used to be here too, on the reasoning that a push clears it. It +// does, but a push is not always the right next move — reviews still running, a +// question to the user still open — and the line held every one of those turns +// open until it was obeyed. It moved to SessionStart beside drift. // // Drift used to be here and is not any more, for exactly that reason: "source // changed on this branch with no box ticked" is a fact about the branch, not @@ -265,9 +276,9 @@ func unattendedLines() []string { // agent — correctly — kept answering that the work was not orphaned. It moved to // SessionStart, which is said once and cannot hold a turn open. // -// Both halves are silent when they have nothing to say, and that is the property -// to protect: sections that each speak sometimes are a report worth reading, and -// sections that always speak are a banner. +// It is silent when it has nothing to say, and that is the property to protect: a +// stage that speaks sometimes is a report worth reading, and one that always +// speaks is a banner. func stopContext(root string) []string { var out []string if set := stopFindings(root); set != nil && !set.Empty() { @@ -275,9 +286,6 @@ func stopContext(root string) []string { out = append(out, stopFindingLines(set)...) out = append(out, fmt.Sprintf("Run `%s validate` for the rest, and fix these before the commit.", prog())) } - if line := undeliveredLine(root); line != "" { - out = append(out, line) - } return out } @@ -364,17 +372,14 @@ func stopFindingLines(set *finding.Set) []string { return out } -// undeliveredLine says that finished work has not left the machine, and stops -// there. It names the commands rather than running them: publishing is the -// agent's act, in its next turn, where a person can see it happen. +// undeliveredLine says that committed work has not left the machine, and stops +// there. It states a fact and points at the sequence rather than naming a push: +// whether the work is finished, and whether the reviews have run, is not +// something the tree can tell. // -// **Finished is the word that does the work, and a dirty tree is not it.** Said on -// the first commit of a unit of work, this asks for a push in the middle of it — -// and because Stop is a continuation, the ask is acted on: a push per turn, each -// one paying for the pre-push gate's build, lint and suite, and a pull request -// opened on a third of a feature. Uncommitted changes are the cheapest available -// evidence that the work is still in hand, so the line waits for them to be gone. -// A clean tree with commits the remote does not have is the state this is for. +// A dirty tree says nothing: uncommitted changes are the cheapest evidence that +// the work is still in hand. A clean tree with commits the remote does not have +// is the state this is for. func undeliveredLine(root string) string { if git.Dirty(root) { return "" @@ -388,10 +393,10 @@ func undeliveredLine(root string) string { what += "s" } if !u.Pushed { - return fmt.Sprintf("scc: `%s` has %s and has never been pushed — `git push -u origin %s`, then open the PR per delivery.md.", - u.Branch, what, u.Branch) + return fmt.Sprintf("scc: `%s` has %s never pushed. When its work is done, deliver it per delivery.md: reviews, then push and the PR.", + u.Branch, what) } - return fmt.Sprintf("scc: `%s` has %s that origin does not have — `git push`, then check the PR per delivery.md.", + return fmt.Sprintf("scc: `%s` has %s that origin does not have. When its work is done, deliver it per delivery.md: reviews, then push and the PR.", u.Branch, what) } diff --git a/internal/cli/hooks_drift_test.go b/internal/cli/hooks_drift_test.go index ab4b291..c0781da 100644 --- a/internal/cli/hooks_drift_test.go +++ b/internal/cli/hooks_drift_test.go @@ -381,11 +381,11 @@ func TestAHostilePathIsRenderedAsOneOrdinaryLine(t *testing.T) { func TestStopIsSilentWhileTheHarnessIsAlreadyContinuing(t *testing.T) { root := driftRepo(t) gitDo(t, root, "checkout", "-q", "-b", "feat/x") - write(t, root, "internal/thing/thing.go", "package thing\n") - // Committed but never pushed, so the Stop stage genuinely has something to - // say. Without that this test passes for the wrong reason — a silent stage is - // silent whether or not the flag is honoured, and the first cut of this proved - // exactly nothing. + // A plan this branch wrote with a heading the format does not allow, so the + // Stop stage genuinely has something to say. Without that this test passes for + // the wrong reason — a silent stage is silent whether or not the flag is + // honoured, and the first cut of this proved exactly nothing. + write(t, root, "plans/broken.md", "# Broken\n\nA plan.\n\n## Notes\n\nnot a section this format allows.\n") gitDo(t, root, "add", "-A") gitDo(t, root, "commit", "-q", "-m", "feat: a thing") @@ -443,3 +443,37 @@ func TestDriftIsReportedWhereItCanBeHeardOnce(t *testing.T) { t.Errorf("Stop still carries a fact the next turn cannot resolve: %q", stop) } } + +// Unpushed work is said once, at SessionStart, and never at Stop. At Stop it read +// as an order the next turn obeyed — a push while reviews were still running or a +// question was still open — and it came back at the end of every turn spent +// waiting. The line points at delivery.md's sequence rather than at a bare push. +func TestUnpushedWorkIsReportedAtSessionStartOnly(t *testing.T) { + root := driftRepo(t) + gitDo(t, root, "checkout", "-q", "-b", "feat/x") + write(t, root, "internal/thing/thing.go", "package thing\n") + gitDo(t, root, "add", "-A") + gitDo(t, root, "commit", "-q", "-m", "feat: a thing") + + start, _, code := runStdin(t, `{"hook_event_name":"SessionStart"}`, + "hooks", "run", "session-start", "--root", root) + if code != ExitOK { + t.Fatalf("session-start exit = %d", code) + } + ctx := additionalContext(t, start) + if !strings.Contains(ctx, "`feat/x` has 1 commit never pushed") { + t.Errorf("SessionStart does not report the unpushed branch: %q", ctx) + } + if !strings.Contains(ctx, "reviews, then push") { + t.Errorf("the line skips the reviews delivery.md puts before the push: %q", ctx) + } + + stop, _, code := runStdin(t, `{"hook_event_name":"Stop","stop_hook_active":false}`, + "hooks", "run", "stop", "--root", root) + if code != ExitOK { + t.Fatalf("stop exit = %d", code) + } + if strings.Contains(stop, "never pushed") { + t.Errorf("Stop still asks for a push: %q", stop) + } +} diff --git a/internal/hooks/harness.go b/internal/hooks/harness.go index 47da371..5e95dab 100644 --- a/internal/hooks/harness.go +++ b/internal/hooks/harness.go @@ -81,11 +81,11 @@ func (e Event) Stage() Stage { func (e Event) Why() string { switch e { case SessionStart: - return "syncs the graph, reports drift, says when nobody is watching, and names what this workspace is missing" + return "syncs the graph, reports drift and unpushed work, says when nobody is watching, and names what this workspace is missing" case UserPromptSubmit: return "says how to read an artifact the prompt named, and nothing otherwise" case Stop: - return "syncs the graph, then reports findings and undelivered work" + return "syncs the graph, then reports findings" } return "" } diff --git a/plans/undelivered-at-session-start.md b/plans/undelivered-at-session-start.md new file mode 100644 index 0000000..8fbffa1 --- /dev/null +++ b/plans/undelivered-at-session-start.md @@ -0,0 +1,37 @@ +--- +autonomy: auto +ci: wait +--- + +# Undelivered at session start + +Move the "commits never pushed" line from the Stop hook to SessionStart, and word it +as a pointer to the delivery sequence rather than an order to push. + +## Why + +On Stop, context continues the conversation, so the line read as an instruction and +re-fired at the end of every turn spent waiting — on the review agents, on a question +to the user, on a branch nobody wanted pushed yet — and it asked for a push before the +reviews delivery.md puts first. Done when Stop no longer reports unpushed work, +SessionStart does, and the suite and lint pass. + +## Paths + +- `internal/cli/hooks_agent.go` +- `internal/hooks/harness.go` +- `docs/wiki/pages/hooks.md` +- `README.md` + +## Tasks + +- [x] 1.1 (Unit) Report unpushed work at SessionStart instead of Stop, pointing at delivery.md's reviews-then-push sequence +- [x] 1.2 (Unit) Give the Stop flag regression test a finding to say now that unpushed work is gone from Stop + _Depends 1.1_ +- [x] 1.3 (Unit) Update the hooks wiki page, the README and the stage descriptions + _Depends 1.1_ + +## Done when + +- `go test ./...` and `golangci-lint run` exit 0. +- `scc validate --checks --pr` exits 0. From baae31eca4628a14c4d0e8ba35b0a84a358d5200 Mon Sep 17 00:00:00 2001 From: prode Date: Sun, 18 Oct 2026 02:34:04 -0300 Subject: [PATCH 2/2] docs(hooks): say emitHook stays silent per turn and per session --- internal/cli/hooks_agent.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/cli/hooks_agent.go b/internal/cli/hooks_agent.go index 655b2f7..4ead51d 100644 --- a/internal/cli/hooks_agent.go +++ b/internal/cli/hooks_agent.go @@ -139,8 +139,8 @@ type hookSpecific struct { // emitHook writes the document, or nothing at all. // // Nothing at all is the common case and the important one: a workspace with no -// findings and nothing undelivered has no business adding a line to every turn of -// every session. Silence is what keeps the line that does appear worth reading. +// findings, no drift and nothing undelivered has no business adding a line to a +// turn or a session. Silence is what keeps the line that does appear worth reading. func emitHook(stage hooks.Stage, lines []string) int { if len(lines) == 0 { return ExitOK