diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 0d16305..91c2aa2 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -85,7 +85,7 @@ jobs: run: go test -v -timeout=15m ./... - name: Build - run: go build ./cmd/no-mistakes + run: go build ./cmd/no-slop e2e: runs-on: ubuntu-latest @@ -96,7 +96,7 @@ jobs: with: go-version-file: go.mod - # The e2e suite drives the real no-mistakes binary against a fake + # The e2e suite drives the real no-slop binary against a fake # agent through `git push -> daemon -> pipeline -> push to upstream` # for claude, codex, and opencode. It builds the binary itself, so # no separate build step is needed. Linux-only for now: opencode's diff --git a/.github/workflows/no-mistakes-required.yml b/.github/workflows/no-slop-required.yml similarity index 70% rename from .github/workflows/no-mistakes-required.yml rename to .github/workflows/no-slop-required.yml index a2bdb8f..3a36da4 100644 --- a/.github/workflows/no-mistakes-required.yml +++ b/.github/workflows/no-slop-required.yml @@ -1,4 +1,4 @@ -name: Require no-mistakes +name: Require no-slop run-name: "PR #${{ github.event.pull_request.number }} body compliance - ${{ github.event.action }} - event ${{ github.run_number }} (run ${{ github.run_id }})" on: @@ -21,42 +21,45 @@ permissions: # an immutable per-event group so first-time-fork approvals can never collapse # opened/edited checks. Keep synchronize/reopened coalescing as before. concurrency: - group: no-mistakes-required-${{ github.event.pull_request.number }}-${{ (github.event.action == 'opened' || github.event.action == 'edited') && github.run_id || 'head-change' }} + group: no-slop-required-${{ github.event.pull_request.number }}-${{ (github.event.action == 'opened' || github.event.action == 'edited') && github.run_id || 'head-change' }} cancel-in-progress: true jobs: check: - name: PR must be raised via no-mistakes + name: PR must be raised via no-slop runs-on: ubuntu-latest # Known automation accounts are exempt so the release pipeline keeps working: # - github-actions[bot] opens the release-please PR via GITHUB_TOKEN # - dependabot[bot] opens dependency update PRs - # Other authors (human or bot) must raise PRs through `git push no-mistakes`. + # Other authors (human or bot) must raise PRs through `git push no-slop`. if: >- github.event.pull_request.user.login != 'github-actions[bot]' && github.event.pull_request.user.login != 'dependabot[bot]' && github.event.pull_request.user.login != 'release-please[bot]' steps: - - name: Verify no-mistakes signature in PR body + - name: Verify no-slop signature in PR body env: PR_BODY: ${{ github.event.pull_request.body }} PR_AUTHOR: ${{ github.event.pull_request.user.login }} PR_NUMBER: ${{ github.event.pull_request.number }} run: | set -eu - marker='Updates from [git push no-mistakes](https://github.com/kunchenguid/no-mistakes)' - if printf '%s' "${PR_BODY:-}" | grep -qF -- "$marker"; then - echo "Found no-mistakes signature in PR #${PR_NUMBER} body." + canonical_marker='Updates from [git push no-slop](https://github.com/Blakeolson21/no-slop)' + legacy_marker='Updates from [git push no-mistakes](https://github.com/Blakeolson21/no-slop)' + if printf '%s' "${PR_BODY:-}" | grep -qF -- "$canonical_marker" || + printf '%s' "${PR_BODY:-}" | grep -qF -- "$legacy_marker"; then + echo "Found no-slop signature in PR #${PR_NUMBER} body." exit 0 fi { - echo "::error::This PR was not raised through no-mistakes." + echo "::error::This PR was not raised through no-slop." echo - echo "Contributions to this repository must be submitted via 'git push no-mistakes'." + echo "Contributions to this repository must be submitted via 'git push no-slop'." echo "That pipeline runs the required review/test/lint/CI steps and writes a" - echo "deterministic '## Pipeline' section into the PR body containing:" + echo "deterministic '## Pipeline' section into the PR body containing one of:" echo - echo " $marker" + echo " $canonical_marker" + echo " $legacy_marker" echo echo "See CONTRIBUTING.md for setup and the full workflow." echo diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index db0dc41..32551b3 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -68,8 +68,8 @@ jobs: DATE="$(date -u +%Y-%m-%dT%H:%M:%SZ)" COMMIT="$(git rev-parse --short=7 HEAD)" CGO_ENABLED=0 GOOS="$GOOS" GOARCH="$GOARCH" \ - go build -ldflags "-X github.com/kunchenguid/no-mistakes/internal/buildinfo.Version=${TAG} -X github.com/kunchenguid/no-mistakes/internal/buildinfo.Commit=${COMMIT} -X github.com/kunchenguid/no-mistakes/internal/buildinfo.Date=${DATE} -X github.com/kunchenguid/no-mistakes/internal/buildinfo.TelemetryHost=${UMAMI_HOST} -X github.com/kunchenguid/no-mistakes/internal/buildinfo.TelemetryWebsiteID=${UMAMI_WEBSITE_ID}" \ - -o "dist/no-mistakes" ./cmd/no-mistakes + go build -ldflags "-X github.com/Blakeolson21/no-slop/internal/buildinfo.Version=${TAG} -X github.com/Blakeolson21/no-slop/internal/buildinfo.Commit=${COMMIT} -X github.com/Blakeolson21/no-slop/internal/buildinfo.Date=${DATE} -X github.com/Blakeolson21/no-slop/internal/buildinfo.TelemetryHost=${UMAMI_HOST} -X github.com/Blakeolson21/no-slop/internal/buildinfo.TelemetryWebsiteID=${UMAMI_WEBSITE_ID}" \ + -o "dist/no-slop" ./cmd/no-slop # Import the Developer ID Application cert into an ephemeral keychain locked # with a runtime-generated password, discover exactly one signing identity @@ -87,9 +87,9 @@ jobs: echo "::error::CSC_LINK/CSC_KEY_PASSWORD signing secrets are missing; refusing to publish an unsigned macOS artifact" >&2 exit 1 fi - KEYCHAIN_PATH="$RUNNER_TEMP/nm-signing.keychain-db" + KEYCHAIN_PATH="$RUNNER_TEMP/ns-signing.keychain-db" KEYCHAIN_PASSWORD="$(openssl rand -base64 24)" - CERT_PATH="$RUNNER_TEMP/nm-developer-id.p12" + CERT_PATH="$RUNNER_TEMP/ns-developer-id.p12" echo "KEYCHAIN_PATH=$KEYCHAIN_PATH" >> "$GITHUB_ENV" # Reconstruct the cert from the base64 secret into RUNNER_TEMP only. @@ -127,10 +127,10 @@ jobs: IDENTITY_HASH="$(printf '%s\n' "$IDENTITIES" | awk 'NR==1 {print $2}')" codesign --force --timestamp --options runtime \ - --identifier com.kunchenguid.no-mistakes \ + --identifier com.kunchenguid.no-slop \ --keychain "$KEYCHAIN_PATH" \ --sign "$IDENTITY_HASH" \ - "dist/no-mistakes" + "dist/no-slop" # Strict verification gate: any missing or ambiguous property fails the # release before the artifact is archived or uploaded. @@ -140,7 +140,7 @@ jobs: run: | set -euo pipefail TEAM_ID="9T2J7MNUP9" - BIN="dist/no-mistakes" + BIN="dist/no-slop" case "$GOARCH" in amd64) EXPECTED_ARCH="x86_64" ;; @@ -156,7 +156,7 @@ jobs: printf '%s\n' "$SIG" grep -q 'Authority=Developer ID Application' <<<"$SIG" grep -q "TeamIdentifier=$TEAM_ID" <<<"$SIG" - grep -q 'Identifier=com.kunchenguid.no-mistakes' <<<"$SIG" + grep -q 'Identifier=com.kunchenguid.no-slop' <<<"$SIG" if grep -qi 'adhoc' <<<"$SIG"; then echo "::error::signature is ad-hoc" >&2; exit 1 fi @@ -179,7 +179,7 @@ jobs: # The Team ID starts with a digit so codesign quotes it, but accept the # unquoted form too so a codesign quirk cannot fail a valid release. grep -Eq "leaf\[subject.OU][[:space:]]*=[[:space:]]*\"?$TEAM_ID\"?" <<<"$DR" - grep -q 'identifier "com.kunchenguid.no-mistakes"' <<<"$DR" + grep -q 'identifier "com.kunchenguid.no-slop"' <<<"$DR" if grep -q 'cdhash H' <<<"$DR"; then echo "::error::designated requirement is content-based (cdhash), not identity-based" >&2; exit 1 fi @@ -197,20 +197,26 @@ jobs: TAG: ${{ needs.release-please.outputs.tag_name }} run: | set -euo pipefail - ARCHIVE="dist/no-mistakes-${TAG}-${GOOS}-${GOARCH}.tar.gz" - tar -C dist -czf "$ARCHIVE" no-mistakes + ARCHIVE="dist/no-slop-${TAG}-${GOOS}-${GOARCH}.tar.gz" + LEGACY_ARCHIVE="dist/no-mistakes-${TAG}-${GOOS}-${GOARCH}.tar.gz" + tar -C dist -czf "$ARCHIVE" no-slop + cp dist/no-slop dist/no-mistakes + tar -C dist -czf "$LEGACY_ARCHIVE" no-mistakes echo "ARCHIVE=$ARCHIVE" >> "$GITHUB_ENV" + echo "LEGACY_ARCHIVE=$LEGACY_ARCHIVE" >> "$GITHUB_ENV" - name: Upload release asset env: GH_TOKEN: ${{ github.token }} TAG: ${{ needs.release-please.outputs.tag_name }} - run: gh release upload "$TAG" "$ARCHIVE" --clobber + run: gh release upload "$TAG" "$ARCHIVE" "$LEGACY_ARCHIVE" --clobber - uses: actions/upload-artifact@v7 with: name: archive-${{ matrix.goos }}-${{ matrix.goarch }} - path: ${{ env.ARCHIVE }} + path: | + ${{ env.ARCHIVE }} + ${{ env.LEGACY_ARCHIVE }} # Tear down the ephemeral keychain on success and failure alike. - name: Clean up signing keychain @@ -220,7 +226,7 @@ jobs: if [ -n "${KEYCHAIN_PATH:-}" ]; then security delete-keychain "$KEYCHAIN_PATH" 2>/dev/null || true fi - rm -f "$RUNNER_TEMP/nm-developer-id.p12" 2>/dev/null || true + rm -f "$RUNNER_TEMP/ns-developer-id.p12" 2>/dev/null || true build-and-upload: runs-on: ubuntu-latest @@ -259,37 +265,47 @@ jobs: mkdir -p dist DATE="$(date -u +%Y-%m-%dT%H:%M:%SZ)" COMMIT="$(git rev-parse --short=7 HEAD)" - BIN="no-mistakes" + BIN="no-slop" + LEGACY_BIN="no-mistakes" OUT="dist/${BIN}" if [ "$GOOS" = "windows" ]; then BIN="${BIN}.exe" + LEGACY_BIN="${LEGACY_BIN}.exe" OUT="dist/${BIN}" fi CGO_ENABLED=0 GOOS="$GOOS" GOARCH="$GOARCH" \ - go build -ldflags "-X github.com/kunchenguid/no-mistakes/internal/buildinfo.Version=${TAG} -X github.com/kunchenguid/no-mistakes/internal/buildinfo.Commit=${COMMIT} -X github.com/kunchenguid/no-mistakes/internal/buildinfo.Date=${DATE} -X github.com/kunchenguid/no-mistakes/internal/buildinfo.TelemetryHost=${UMAMI_HOST} -X github.com/kunchenguid/no-mistakes/internal/buildinfo.TelemetryWebsiteID=${UMAMI_WEBSITE_ID}" \ - -o "$OUT" ./cmd/no-mistakes + go build -ldflags "-X github.com/Blakeolson21/no-slop/internal/buildinfo.Version=${TAG} -X github.com/Blakeolson21/no-slop/internal/buildinfo.Commit=${COMMIT} -X github.com/Blakeolson21/no-slop/internal/buildinfo.Date=${DATE} -X github.com/Blakeolson21/no-slop/internal/buildinfo.TelemetryHost=${UMAMI_HOST} -X github.com/Blakeolson21/no-slop/internal/buildinfo.TelemetryWebsiteID=${UMAMI_WEBSITE_ID}" \ + -o "$OUT" ./cmd/no-slop + cp "$OUT" "dist/${LEGACY_BIN}" if [ "$GOOS" = "windows" ]; then - ARCHIVE="dist/no-mistakes-${TAG}-${GOOS}-${GOARCH}.zip" + ARCHIVE="dist/no-slop-${TAG}-${GOOS}-${GOARCH}.zip" + LEGACY_ARCHIVE="dist/no-mistakes-${TAG}-${GOOS}-${GOARCH}.zip" ( cd dist zip -q "$(basename "$ARCHIVE")" "$BIN" + zip -q "$(basename "$LEGACY_ARCHIVE")" "$LEGACY_BIN" ) else - ARCHIVE="dist/no-mistakes-${TAG}-${GOOS}-${GOARCH}.tar.gz" + ARCHIVE="dist/no-slop-${TAG}-${GOOS}-${GOARCH}.tar.gz" + LEGACY_ARCHIVE="dist/no-mistakes-${TAG}-${GOOS}-${GOARCH}.tar.gz" tar -C dist -czf "$ARCHIVE" "$BIN" + tar -C dist -czf "$LEGACY_ARCHIVE" "$LEGACY_BIN" fi echo "ARCHIVE=$ARCHIVE" >> "$GITHUB_ENV" + echo "LEGACY_ARCHIVE=$LEGACY_ARCHIVE" >> "$GITHUB_ENV" - name: Upload release asset env: GH_TOKEN: ${{ github.token }} TAG: ${{ needs.release-please.outputs.tag_name }} - run: gh release upload "$TAG" "$ARCHIVE" --clobber + run: gh release upload "$TAG" "$ARCHIVE" "$LEGACY_ARCHIVE" --clobber - uses: actions/upload-artifact@v7 with: name: archive-${{ matrix.goos }}-${{ matrix.goarch }} - path: ${{ env.ARCHIVE }} + path: | + ${{ env.ARCHIVE }} + ${{ env.LEGACY_ARCHIVE }} checksums: runs-on: ubuntu-latest @@ -314,7 +330,7 @@ jobs: set -euo pipefail ( cd dist - sha256sum no-mistakes-* > ../checksums.txt + sha256sum no-slop-* no-mistakes-* > ../checksums.txt ) - name: Upload checksums diff --git a/.no-mistakes.yaml b/.no-slop.yaml similarity index 98% rename from .no-mistakes.yaml rename to .no-slop.yaml index 682be69..19aca36 100644 --- a/.no-mistakes.yaml +++ b/.no-slop.yaml @@ -1,4 +1,4 @@ -# no-mistakes' own dogfood config. +# no-slop's own dogfood config. # Local Test is intentionally empty: the Test step is targeted validation of the # requested intent (agent-driven smallest relevant checks + evidence), never a # repository-wide regression suite. Broad race-enabled Go coverage stays in diff --git a/AGENTS.md b/AGENTS.md index 40ff3e5..3cdf268 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -2,8 +2,8 @@ This file is for agentic coding tools working in this repo. -This repository is a Go CLI app named `no-mistakes`. -The binary entrypoint is `cmd/no-mistakes`; implementation code lives under `internal/`, and the package names there are the layout map (CLI in `internal/cli`, daemon in `internal/daemon`, pipeline and steps in `internal/pipeline`, agent adapters in `internal/agent`, terminal UI in `internal/tui`, shared infrastructure in `internal/git`, `internal/ipc`, `internal/config`, `internal/db`, `internal/paths`, `internal/types`). +This repository is a Go CLI app named `no-slop`. +The binary entrypoint is `cmd/no-slop`; implementation code lives under `internal/`, and the package names there are the layout map (CLI in `internal/cli`, daemon in `internal/daemon`, pipeline and steps in `internal/pipeline`, agent adapters in `internal/agent`, terminal UI in `internal/tui`, shared infrastructure in `internal/git`, `internal/ipc`, `internal/config`, `internal/db`, `internal/paths`, `internal/types`). Build, test, and release commands are owned by the `Makefile`; read it for the full target list instead of relying on a copy here. Safest local verification sequence after non-trivial changes: @@ -12,12 +12,12 @@ Safest local verification sequence after non-trivial changes: - `make lint` (generated-skill drift check plus `go vet`) - `go test -race ./...` (the e2e suite is behind the `e2e` build tag and excluded) - `make e2e` when touching agent integrations, the e2e harness, or recorded fixtures -- `go build -o ./bin/no-mistakes ./cmd/no-mistakes` +- `go build -o ./bin/no-slop ./cmd/no-slop` **Fork Routing** - `repos.upstream_url` is the parent repository used for PR base routing; `repos.fork_url` is an optional GitHub fork push target. -- `no-mistakes init --fork-url ` expects `origin` to point at the GitHub parent repository and `` at the contributor fork; plain `no-mistakes init` preserves an existing fork URL on idempotent refresh. +- `no-slop init --fork-url ` expects `origin` to point at the GitHub parent repository and `` at the contributor fork; plain `no-slop init` preserves an existing fork URL on idempotent refresh. - Push and CI auto-fix push code must resolve the push URL via `resolvePushURL` (`internal/pipeline/steps/common_git.go`) so configured forks still receive branch updates; the non-fork path recovers the credentialled upstream from the worktree's `origin` remote at run time because the DB `upstream_url` is stored redacted (see Credential Redaction below). `Repo.PushURL()` remains correct only for fork-only callers (e.g. `rebase.go`), since fork URLs carry no embedded credentials. - GitHub PR code must keep `--repo` pointed at the parent and use `--head :` when `fork_url` is set; existing-PR lookup must list by the bare branch and filter head-owner fields, never pass `:` to `gh pr list --head`. - GitLab and Bitbucket fork MR/PR routing is intentionally out of scope until implemented end to end; if a legacy row has `fork_url` for those hosts, PR creation must skip instead of opening a self PR. @@ -39,7 +39,7 @@ Safest local verification sequence after non-trivial changes: - Keep `README.md` concise and high-level; the bar needs to be extremely high for what shows up there. - Most documentation lives in `docs/`, the published docs site. - One owner per fact: `docs/src/content/docs/reference/global-config.md` and `docs/src/content/docs/reference/repo-config.md` own configuration keys, `docs/src/content/docs/reference/environment.md` owns environment variables and the telemetry local/remote split, `docs/src/content/docs/concepts/daemon.md` owns the daemon lifecycle model, and guides pages explain purpose and link to those owners instead of restating tables and examples. -- The `document.instructions` block in `.no-mistakes.yaml` states this ownership map for the pipeline's document step; update it when ownership moves. +- The `document.instructions` block in `.no-slop.yaml` states this ownership map for the pipeline's document step; update it when ownership moves. **NoSlop Front Stage** @@ -50,7 +50,7 @@ Safest local verification sequence after non-trivial changes: **Agent-Guidance Surfaces** -- `skills/no-mistakes/SKILL.md` is **generated**: the source of truth is the `body` constant in `internal/skill/skill.go`. Edit the body, then `make skill`; `make lint` fails CI on drift. Never edit `SKILL.md` directly. `no-mistakes init` ships this rendering to agents at user level. +- `skills/no-slop/SKILL.md` is **generated**: the source of truth is the `body` constant in `internal/skill/skill.go`. Edit the body, then `make skill`; `make lint` fails CI on drift. Never edit `SKILL.md` directly. `no-slop init` ships this rendering to agents at user level. - Agent-driving guidance is owned by the skill body and the live `axi` output strings (`internal/cli/axi*.go`); `docs/src/content/docs/guides/agents.md` carries only the canonical invariant sentences pinned by `internal/cli/axi_guidance_test.go` plus a pointer to the skill. When you change driving guidance, change the skill body and the point-of-use `axi` strings together; that drift test is the sync check. - The shared default test-quality rule lives in `internal/testguidance`; render it only into the task-first skill and pipeline roles that can author, repair, or review tests. Its fake-agent prompt tests are the intentional generated-interface contract, not source-text checks. - Review auto-fix is disabled by default (`auto_fix.review: 0` in `config.go` `autoFixDefaults`), so blocking and ask-user review findings park for an agent decision; keep the skill, the live `axi` gate `note`, and docs qualified if you touch review auto-fix. @@ -66,7 +66,7 @@ Safest local verification sequence after non-trivial changes: **Recursive Gate-Execution Containment** -- `internal/gatecontext` is the single classifier for recursive pipeline control. It combines canonical registered gate common-directory identity with OS-authenticated IPC peer ancestry; `NO_MISTAKES_GATE` is diagnostic only. CLI preflight, daemon mutation ingress, gate init/eject, branch-sync mutation, and the managed pre-receive hook must all keep using that owner so marker removal, cwd changes, and direct pushes cannot bypass refusal. Read-only AXI status/logs, help, and doctor remain available. Regressions: `internal/gatecontext`, `TestGateStepCannotStartRecursivePipeline`. +- `internal/gatecontext` is the single classifier for recursive pipeline control. It combines canonical registered gate common-directory identity with OS-authenticated IPC peer ancestry; `NS_GATE` and its `NO_MISTAKES_GATE` compatibility alias are diagnostic only. CLI preflight, daemon mutation ingress, gate init/eject, branch-sync mutation, and the managed pre-receive hook must all keep using that owner so marker removal, cwd changes, and direct pushes cannot bypass refusal. Read-only AXI status/logs, help, and doctor remain available. Regressions: `internal/gatecontext`, `TestGateStepCannotStartRecursivePipeline`. - Every pipeline agent prompt receives the phase boundary from `internal/gateguidance`, and the generated user-level skill reuses the same owner. Step agents return only their assigned phase; the outer executor alone controls other validation, push, PR, and CI phases. Edit `internal/skill/skill.go`, then run `make skill`; never edit the generated skill directly. **Process-Tree Reaping (`internal/proctree`)** @@ -74,13 +74,13 @@ Safest local verification sequence after non-trivial changes: - A process group is not a process tree. `Setpgid` alone cannot reap a `setsid()` descendant, so `internal/proctree` walks `ppid` links transitively and `shellenv` reaps that union alongside the group kill. Snapshots MUST be taken **before** the leader is signalled: the instant it dies the kernel rewrites its children's `ppid` to 1 and the only trail back to the command is gone. - That ordering is unachievable on the clean-exit path, where `TerminateShellCommandGroup` runs after `cmd.Wait` returned. A poller (`internal/shellenv/tracker_unix.go`, default 15s, one `ps` per tick for all leaders) samples descendants **while the leader is alive** and accumulates a union per leader; that union, not the post-mortem snapshot, is what catches an escapee. Registering a leader pulls the next sample forward to `trackerFirstSample`, because the normal daemon state is a poller already running for some other step; the union drops pids the process table no longer shows, which is what keeps the persisted record and the `ps` argv entry bounded on a long step. `StartShellCommand` registers leaders and `TerminateShellCommandGroup` consumes them, so new call sites are covered without edits - do not move registration to call sites. - Every per-pid kill and process-group kill is guarded by a fresh targeted start-time lookup. A group is killed only when its sampled leader still matches; an unsampled or recycled group leader fails closed. Both kill paths also refuse a protected pid: pid 0 and 1, the current process, its ancestors, and the leader of its own process group. Every `ps` the package runs is itself bounded, because both kill paths run inside `cmd.Cancel`, where `os/exec` has not yet armed the `WaitDelay` backstop. Pids get recycled; an unguarded stale pid or pgid list is a licence to kill strangers. Failing to reap is recoverable, killing the wrong process is not. -- The daemon persists one record per live leader under `/proctrees` (`paths.ProcTreesDir`) and sweeps them in `recoverOnStartup` via `reapOrphanedProcessTrees`, which skips entirely when another daemon is alive. This is the step/agent-subprocess counterpart to `reapOrphanedServers`, which only covers managed servers that write their own PID file. +- The daemon persists one record per live leader under `/proctrees` (`paths.ProcTreesDir`) and sweeps them in `recoverOnStartup` via `reapOrphanedProcessTrees`, which skips entirely when another daemon is alive. This is the step/agent-subprocess counterpart to `reapOrphanedServers`, which only covers managed servers that write their own PID file. - Residual gap, accepted: a process that both spawns and loses its parent inside one tick window is missed. Closing it needs ptrace or a PID namespace. Windows is unaffected - its kill-on-close job object is a stronger guarantee - so `proctree` is unix-only with non-unix stubs. - Regressions: `TestConfigureShellCommand_CancelReapsSetsidEscapedChild`, `TestTerminateShellCommandGroup_ReapsSetsidEscapeeAfterLeaderExits`, `TestNativeAgentCommand_TerminateReapsSetsidEscapeeAfterSampling` (pins the agent path's tracker registration; every other agent reap test keeps its grandchild inside the leader's group, so all of them still pass if `StartShellCommand` is swapped back to a bare `cmd.Start()`), `TestTracker_*`, `TestKill_*`, `TestKillGroups_*`, `TestDescendants_*`, `TestReapRecord_*`, `TestStartTimes_*`, `TestSnapshot_FailsWhenPSDoesNotReturnWithinTheBound`, `TestReapOrphanedProcessTrees_*`. **Filesystem and Paths** -- Use `filepath.Join`; respect `NM_HOME` for app state; directories are `0o755` and files `0o644` by convention. +- Use `filepath.Join`; respect `NS_HOME` for app state; directories are `0o755` and files `0o644` by convention. - On macOS, path comparisons may need symlink resolution (`/var` vs `/private/var`). **Git on Bare Gate Repos (`safe.bareRepository`)** @@ -101,11 +101,11 @@ Safest local verification sequence after non-trivial changes: **Daemon Singleton Lock (`internal/daemon/lock.go`)** -- Only one live daemon may own an `NM_HOME`: an exclusive OS file lock on `/daemon.lock` is acquired as the very first action in `RunWithOptions`, strictly before stale-run recovery and socket bind, and held for the process lifetime. The kernel releases it on any process death, so a held lock always means a live holder and no staleness heuristic is needed. Without it, a second daemon stole the socket and ran global crash recovery against the live daemon's runs and worktrees. +- Only one live daemon may own an `NS_HOME`: an exclusive OS file lock on `/daemon.lock` is acquired as the very first action in `RunWithOptions`, strictly before stale-run recovery and socket bind, and held for the process lifetime. The kernel releases it on any process death, so a held lock always means a live holder and no staleness heuristic is needed. Without it, a second daemon stole the socket and ran global crash recovery against the live daemon's runs and worktrees. - Process launch is not readiness: the PID record is published after the singleton lock and before exclusive recovery, while startup succeeds only after a real IPC health response. The 45s production budget covers cold environment setup and recovery; early exits fail promptly, timeout cleanup reaps detached children before fallback or rollback, and managed plus detached failures retain both causes. Regressions: `TestStartDetachedDaemonDetectsChildExitPromptly`, `TestStartDetachedDaemonTimeoutKillsAndReapsChild`, `TestStartPreservesManagedAndDetachedFallbackErrors`, `TestColdDetachedStartupProductionGateCardinality`. - A successful stop means the daemon process is gone, not merely that IPC health has disappeared, because only process exit releases the singleton lock. Capture the daemon instance before requesting shutdown, and close the shutdown client before waiting because the daemon drains in-flight handlers during exit. A daemon that already died uncleanly is the one stop that must not wait: `recordedDaemonProvablyGone` (`selfexec.go`) is the single owner of that proof - signal-0 process death plus no listener on a dialable endpoint - and both stop paths answer there so they cannot disagree. Never prove death by inspecting the pid (`ps -p` exits 1 for a nonexistent pid, which read as an inspection error and made `daemon restart` refuse to clean up after a crash). See `waitForDaemonStop` and `stopDetachedDaemon`; regressions: `TestWaitForDaemonStopCleansStaleArtifactsOfDeadDaemon`, `TestProvablyStoppedSeparatesDeadDaemonsFromUnreachableOnes`, e2e `TestDaemonStopLeavesNoDaemonProcessOwningTheRoot`, `TestDaemonRestartReplacesTheDaemonWithExactlyOneOwner`. - Independent layers: `internal/ipc` `listen()` dials the socket before unlinking it and refuses to steal a live one; client probes bound the dial with `daemon_connect_timeout` and fail fast on a dead or wedged socket instead of starting a replacement daemon (`EnsureDaemon` surfaces the error with a `daemon start` recovery hint; the health RPC itself is bounded separately by `ipc.DefaultDialTimeout`). -- Daemon execution is explicit-only (`no-mistakes daemon run --root`); never let inherited environment reinterpret probes like `--version` or `status` as daemon workers. +- Daemon execution is explicit-only (`no-slop daemon run --root`); never let inherited environment reinterpret probes like `--version` or `status` as daemon workers. - Startup worktree cleanup is DB-aware: never remove a worktree whose run row is `pending` or `running`; `startRun` inserts the run row before creating the worktree, so a no-row directory is safe to remove immediately. - The user-facing model lives in `docs/src/content/docs/concepts/daemon.md`; the lock rationale lives in the `internal/daemon/lock.go` and `daemon.go` comments. Regressions: `TestAcquireSingletonLock_*`, `TestRunWithResources_SecondDaemonForSameRootFailsWithoutStealingSocket`, `TestRunWithOptions_RequiresSingletonLockBeforeRecovery`, `TestRecoverOnStartup_DoesNotDeleteActiveRunWorktree`, `TestServe_SecondListenerForLiveSocketDoesNotStealIt`, `TestDialConnectTimeoutFailsFastAndNamesSocket`, `TestIsRunningFailsFastWhenSocketAcceptsButDoesNotRespond`, `TestIsRunningSurfacesExistingDeadSocket`, `TestDaemonRunRootFromArgs_EnvDoesNotForceDaemonModeForProbes`, `TestValidateDaemonPIDFallback_RefusesToKillOwnProcess`. @@ -127,24 +127,24 @@ Safest local verification sequence after non-trivial changes: - `daemon stop`, `daemon restart`, and `update` refuse by default while pending/running runs exist (the daemon is machine-wide, so stopping it can fail every active pipeline) and require an explicit `--force`. `update` lists the runs via the shared `lifecycle.ActiveRuns`/`lifecycle.RunList` helpers; stop and restart use the classification helpers below. `update -y` answers only the different-executable prompt and deliberately does not bypass this guard. Self-update is disabled in this fork, so `update` now returns `update.ErrSelfUpdateDisabled` before reaching its guard; keep the guard and its tests intact, because the update machinery they protect is retained, not deleted. - `daemon stop`/`daemon restart` classify each active run with `lifecycle.ClassifyActiveRuns` before deciding and render it with `lifecycle.ActiveRunList`/`ExecutingRunList`. `--force` covers only `parked` and `idle` runs; a run `executing` a step needs `--abandon-executing-runs`, because stopping the daemon mid-step fails the run and strands its pipeline commits in the local gate. Classification is fail-closed on purpose: a live daemon plus a run with no positive parked/idle evidence counts as executing (including a `pending` row, which is startRun's setup window and not a queue), and only a daemon that is not running makes every row idle - either a clean negative health answer, or `daemon.ProvablyStopped` positive proof of death, which is the one thing that outranks the fail-closed assumption because an unclean death is precisely the shape that makes the health probe error. Do not widen `--force` back over executing runs. - `daemon start` carries the executing tier of the same guard, because with a daemon already running it refreshes a drifted managed service definition by stopping and restarting it. It deliberately has no `--force`: parked and idle rows must never refuse the command an operator uses to recover a dead daemon, and its health probe is read strictly (`err == nil && alive`) because only a healthy daemon reaches the destructive branch inside `daemon.Start`. -- Every invocation of the four commands is logged with caller attribution (PID, PPID, parent command line) via `logLifecycleInvocation` to `/logs/cli.log`; this is the incident forensic trail, do not remove or weaken it. `force` and `abandon_executing` are separate fields on that line and must never be collapsed into one boolean: they authorize materially different destruction, and telling them apart afterwards is the whole point. +- Every invocation of the four commands is logged with caller attribution (PID, PPID, parent command line) via `logLifecycleInvocation` to `/logs/cli.log`; this is the incident forensic trail, do not remove or weaken it. `force` and `abandon_executing` are separate fields on that line and must never be collapsed into one boolean: they authorize materially different destruction, and telling them apart afterwards is the whole point. - Regressions: `TestDaemonStopRefusesWithActiveRunsAndListsThem`, `TestDaemonStopForceOverridesActiveRunGuard`, `TestDaemonStopForceRefusesRunExecutingAStep`, `TestDaemonRestartForceRefusesRunExecutingAStep`, `TestDaemonStopForceRefusesPendingRunUnderLiveDaemon`, `TestDaemonStopForceAllowsRunParkedAtAGate`, `TestDaemonStopAbandonExecutingRunsOverridesExecutingGuard`, `TestDaemonStopForceAllowsLeftoverRunsWhenDaemonIsDown`, `TestDaemonRestartRefusesWithActiveRuns`, `TestDaemonStartRefusesRunExecutingAStep`, `TestDaemonStartProceedsWhenNoHealthyDaemonOwnsTheLeftoverRuns`, `TestDaemonRestartAfterUncleanDeathDoesNotDemandAbandoningExecutingRuns`, `TestLifecycleCommandsWriteCallerAttributionToCLILog` (`internal/cli/daemon_lifecycle_test.go`), `TestUpdaterRunRefusesWithActiveRunsAndListsThem`, `TestUpdaterActiveRunGuardAllowsForce` (`internal/update`). **Testing Conventions** - Prefer e2e tests for behavior that crosses a process or I/O boundary (CLI flags, config loading, git operations, agent spawning, daemon coordination, stdout/stderr, recorded fixtures); unit-test pure helpers where speed and failure localization matter. Prefer creating real git repos in temp dirs over heavy mocking. - The e2e suite is behind the `e2e` build tag; `make e2e` runs `scripts/e2e.sh`, which sweeps `./internal/e2e/...` and `./internal/pipeline/steps/...`, so keep new step-local e2e tests behind the tag too. -- Temporary e2e daemons (`NM_TEST_START_DAEMON=1` / harness) are owned by `internal/e2edaemon`: exact inventory, concurrency cap (`NM_E2E_DAEMON_MAX`, default 2), bounded argv checks, and reapers in harness Cleanup, package `TestMain`, and `scripts/e2e.sh` EXIT/INT/TERM. A SIGKILL of the wrapper shell does not run its trap; next-run inventory recovery covers that. External sleep-loop keepalives are out of scope. Never point inventory reaping at the shared `~/.no-mistakes` service. Regressions: `internal/e2edaemon/*_test.go`. +- Temporary e2e daemons (`NS_TEST_START_DAEMON=1` / harness) are owned by `internal/e2edaemon`: exact inventory, concurrency cap (`NS_E2E_DAEMON_MAX`, default 2), bounded argv checks, and reapers in harness Cleanup, package `TestMain`, and `scripts/e2e.sh` EXIT/INT/TERM. A SIGKILL of the wrapper shell does not run its trap; next-run inventory recovery covers that. External sleep-loop keepalives are out of scope. Never point inventory reaping at the shared `~/.no-mistakes` service. Regressions: `internal/e2edaemon/*_test.go`. - Packages whose tests shell out to git unset `GIT_CONFIG_COUNT` in `TestMain` so ambient `GIT_CONFIG_*` injection from agent harnesses cannot leak in; a test exercising injected config re-sets it with `t.Setenv` (see `internal/git`, `internal/gate`, `internal/daemon`, `internal/pipeline/steps`). -- Packages whose tests can start a daemon or touch ambient state (`cmd/no-mistakes`, `internal/cli`, `internal/update`) use a package-wide `TestMain` that points `NM_HOME` and `HOME` at fresh temp dirs and disables telemetry/update-check env vars, so a full test run never touches a real `~/.no-mistakes`. Follow the same pattern in new such packages. -- `paths.New()` refuses the default `~/.no-mistakes` root under `go test`; tests that touch app state must set `NM_HOME` to a temp dir, and only the production-default path test may opt in with `NO_MISTAKES_ALLOW_DEFAULT_ROOT_IN_TESTS=1`. +- Packages whose tests can start a daemon or touch ambient state (`cmd/no-slop`, `internal/cli`, `internal/update`) use a package-wide `TestMain` that points `NS_HOME` and `HOME` at fresh temp dirs and disables telemetry/update-check env vars, so a full test run never touches a real `~/.no-mistakes`. Follow the same pattern in new such packages. +- `paths.New()` refuses the default `~/.no-mistakes` root under `go test`; tests that touch app state must set `NS_HOME` to a temp dir, and only the production-default path test may opt in with `NS_ALLOW_DEFAULT_ROOT_IN_TESTS=1`. - Isolate filesystem and environment state with `t.TempDir()` and `t.Setenv()`. - The Windows CI leg is process-spawn bound, not compute bound: git-backed packages cost roughly 10x their Linux time (`internal/git` 5.7s -> 53s, `internal/branchsync` 31s -> 415s), so the job's wall floor is the slowest single package. Keep long git-heavy packages off the serial critical path (`internal/branchsync` runs `t.Parallel()` for exactly that reason) and keep the Defender scan-exclusion step in `ci.yml`, whose comment owns the rationale. Regressions: `TestCIWorkflow_WindowsTestsRunWithScanExclusions`, `TestCIWorkflow_WindowsHangSurfacesAsGoTimeoutNotJobCancellation`. - Go applies an implicit GOOS constraint from a filename suffix, so a test file named `*_windows_test.go` (or `_linux`, `_darwin`) silently compiles only on that platform. Name platform-agnostic tests about Windows something else. **Repo Config Trust Boundary (security)** -- The daemon runs `commands.*` from `.no-mistakes.yaml` verbatim via `sh -c`, and `agent` selects which process launches with the maintainer's credentials. The code-executing selection fields (`commands.{test,lint,format}` and `agent`) are therefore loaded from the trusted default branch at a **pinned SHA** resolved by a fresh fetch, never from the pushed SHA. The run aborts when the trusted commit or its present config cannot be read and parsed; a readable tree with no config is valid. See `internal/daemon/manager.go` `startRun`, `loadTrustedRepoConfig`, and `assertGateTrustedConfigReadable`. +- The daemon runs `commands.*` from `.no-slop.yaml` verbatim via `sh -c`, and `agent` selects which process launches with the maintainer's credentials. The code-executing selection fields (`commands.{test,lint,format}` and `agent`) are therefore loaded from the trusted default branch at a **pinned SHA** resolved by a fresh fetch, never from the pushed SHA. The run aborts when the trusted commit or its present config cannot be read and parsed; a readable tree with no config is valid. See `internal/daemon/manager.go` `startRun`, `loadTrustedRepoConfig`, and `assertGateTrustedConfigReadable`. - `document.instructions` (the repo's documentation placement policy), `review.path_instructions` (path-scoped review guidance appended to the review prompt), `disable_project_settings` (the gate-agent project-instruction opt-out), `no_ci` (positive declaration that the repository intentionally has no CI), and `ci.rerun_transient` (how many times a transiently failed check may be re-run) are also trusted-only, regardless of `allow_repo_commands`: a pushed branch must not weaken any of those boundaries, self-declare no-CI to bypass checks, or steer its own review; enabling the commands opt-in must not drop the maintainer's own trusted values; and every re-run `ci.rerun_transient` authorizes bills another provider-side workflow run to the repository, so a contributor must not be able to raise it (the operator's own global `ci.rerun_transient` is a separate, non-contributor surface that the trusted repo value still overrides). When the opt-out is enabled, only adapters with verified effective suppression may launch. Other non-executing fields (`ignore_patterns`, `auto_fix`, `commit`, `intent`, `test`) are still read from the pushed branch. - Selecting which trusted config applies to a run must never depend on a pushed-branch field. `review.path_instructions` is matched against the COMPLETE changed-file set, never the `ignore_patterns`-filtered subset, because filtering there lets a contributor suppress a maintainer's rule from their own review by ignoring its glob. `reviewablePaths` (`internal/pipeline/steps/common_diff.go`) answers only "does this run have anything to work on". - `allow_repo_commands` is per-repo, read only from the trusted default-branch copy, and defaults `false`; a contributor cannot self-enable it from a pushed branch. The e2e harness models a trusted single-developer environment and commits `allow_repo_commands: true` via `SetupOpts.AllowRepoCommands`; security tests pass `false`. @@ -154,7 +154,7 @@ Safest local verification sequence after non-trivial changes: - `ci_timeout` is an idle timeout, not an absolute deadline: only `timeoutAnchor` re-arms when the upstream default-branch tip advances, `started` stays fixed for poll pacing, and re-arm only ever extends the deadline (fail-safe on transient base-tip failures). Value semantics (`0` unset, negative unlimited sentinel, keyword parsing) live in `config.go`; keep `config.DefaultCITimeout` and `defaultConfigYAML` in sync (`TestDefaultConfigYAML_MatchesGoDefaults`). User-facing semantics are owned by `docs/src/content/docs/reference/global-config.md`. - CI readiness never treats an unproven empty forge check list as green. Ready requires observed all-green checks, or trusted default-branch `no_ci: true` with zero registered checks (`internal/pipeline/steps/ci.go` decides whether the declaration applies; `internal/cimonitor` owns the agent-facing log vocabulary and Ready/DeclaredNoCI parse). Delayed registration, pending checks, failures, errors, unknowns, and stale-head evidence stay not-ready; registered checks on a declared no-CI repo are still honored. Regressions: `TestChecksPassed_PR607RealLogSequence`, `TestCIStep_EmptyChecksWithoutNoCIStaysNotReadyPastOldGracePeriod`, `TestCIStep_EmptyChecksWithTrustedNoCIBecomesReady`, `TestCIStep_DelayedCheckRegistrationStaysNotReadyUntilGreen`, `TestCIStep_DeclaredNoCIWithUnexpectedChecksHonorsThem`, `TestEffectiveRepoConfig_NoCITrustedOnly`. -- Reap an orphaned monitor from outside its worktree with `no-mistakes axi abort --run `; it needs only `NM_HOME` and never starts a stopped daemon. A known run succeeds only with durable terminal truth, a recorded nonterminal run fails unconfirmed, and only an unknown id is an idempotent no-op. Bare `axi abort` stays worktree/branch-scoped. +- Reap an orphaned monitor from outside its worktree with `no-slop axi abort --run `; it needs only `NS_HOME` and never starts a stopped daemon. A known run succeeds only with durable terminal truth, a recorded nonterminal run fails unconfirmed, and only an unknown id is an idempotent no-op. Bare `axi abort` stays worktree/branch-scoped. - A merged or closed PR observation transactionally completes an active run and its CI step; PR lifecycle state is monotonic, so duplicate or delayed observations cannot reactivate or regress a terminal run. Startup reconciles legacy `pending` or `running` rows that already hold terminal PR state before parked-run planning and generic crash recovery. Regressions: `TestUpdateRunPRStateFinalizesActiveTerminalOutcomes`, `TestUpdateRunPRStateIgnoresDuplicateAndDelayedRegressions`, `TestReconcileTerminalPRRunsFinalizesLegacyActiveRows`, `TestRecoverOnStartup_FinalizesLegacyTerminalPRRun`, e2e `TestTerminalPRRunDisappearsFromActiveListing`. - A provider-reported `cancelled` check is never a job verdict, so the deterministic rerun runs strictly before any CI fix round: it is the only outcome that earns a rerun, a check cancelled again after its budget parks as `ask-user` instead of entering the `auto_fix.ci` loop, and any genuine or unrecognized failure or merge conflict in the same poll suppresses reruns so real failures still escalate on their first observation. The budget is per check name per run and spent on request rather than on success, and a rerun is never issued once the published branch head no longer equals `runs.head_sha`, because it would certify a commit this run never delivered. Each outstanding rerun records its verified pipeline head and the same-name provider links visible when it was requested; it retires durably when the run head advances or a new conclusive non-cancel link appears. Retirement keeps the spent budget and never changes check buckets. A delayed same-named green sibling can satisfy the link trigger on the same head, matching the default branch's existing name-keyed masking; removing that limitation requires provider truth outside this policy. Classification, the deliberate `TIMED_OUT`/`STALE` exclusions, rollup-lag grace, and retirement live in `internal/pipeline/steps/ci_transient.go`; provider support is the optional `scm.CheckRerunner` (GitHub only) and user-facing semantics are owned by `docs/src/content/docs/reference/repo-config.md`. Regressions: `TestCIStep_CancelledCheckIsRerunBeforeEscalating`, `TestCIStep_CancelledCheckStaysUnresolvedAfterItsBudget`, `TestCIStep_LaggingRerunRollupKeepsWaitingForTheRepublishedCheck`, `TestCIStep_SameHeadGreenRerunEmitsChecksPassed`, `TestCIStep_DelayedSameNameCheckRetainsLegacyNameBehavior`, `TestCIStep_ResolvedRerunDoesNotParkALaterGreenHead`, `TestRetireResolvedReruns`, `TestRetireResolvedRerunsRetriesAfterPersistenceFailure`, `TestCIStep_MovedPublishedHeadTerminatesInsteadOfRerunning`, `TestCIStep_MovedPublishedHeadClearsCIReadiness`, `TestClassifyCheckFailure`. - **Terminal is not pending.** Readiness must reject every non pass/fail/skip bucket (`hasUnresolvedChecks`), but only checks that can still finish on their own (`hasPendingChecks`) may keep the monitor polling. A `cancel` bucket - GitHub `CANCELLED`, GitLab `canceled`, Bitbucket `STOPPED`, and how GitHub reports a job killed by its own `timeout-minutes` - is a published conclusion that nothing will replace, so with no rerun outstanding it parks at `ciUnresolvedCancelledOutcome` (`cancelledWithoutRerun`) instead of waiting. Conflating the two is the #628 regression that hung real runs for their whole `ci_timeout`; an unrecognized bucket is deliberately still treated as waiting, because unknown is not evidence of terminal. Regressions: `TestCIStep_CancelledCheckAmongPassingChecksEscalatesInsteadOfPollingForever`, `TestCIStep_ZeroRerunBudgetEscalatesCancelledCheckWithoutMakingItReady`, `TestCIStep_BitbucketStoppedCheckParksForADecision`. @@ -168,9 +168,9 @@ Safest local verification sequence after non-trivial changes: **Agent Lane Quota Cooldown (`internal/lanehealth`)** - `lanehealth` is the single owner of "which configured agent lane is quota-exhausted, and until when". `agent.WithLaneHealth` wraps each lane inside `newLaneAgents` (`internal/daemon/manager.go`, the one owner both pipeline-agent construction sites use), so a marked lane is skipped before any process launches, a quota banner on a failed invocation marks the lane, and a successful invocation clears any mark observed no later than the moment that invocation started (`ClearObservedBefore`), so a mark a concurrent run wrote mid-invocation stays sticky. A mark is not sealed until its reset time: `ClaimProbe` lets one invocation per `ProbeInterval` (1h) through, so a stated multi-day reset - trusted from a single observation, with the only evidence that could correct it being exactly what the mark suppresses - self-corrects within an hour of the account actually recovering. `fallbackAgent` treats a `*LaneOutageError` as a fallback trigger and, when EVERY lane failed that way, returns one aggregate error naming each lane's reset time rather than only the last banner. -- Marks live in `/lane-health.json` (atomic rename, `internal/filelock` for the read-modify-write), because the failure being fixed was a dozen separate runs each paying an agent spawn to rediscover the same dead account. +- Marks live in `/lane-health.json` (atomic rename, `internal/filelock` for the read-modify-write), because the failure being fixed was a dozen separate runs each paying an agent spawn to rediscover the same dead account. - Classification is deliberately narrow and only ever reads a FAILED invocation's error text, which the adapters build from provider stderr and the structured error channel, never agent-authored stdout: both CLIs ship unrelated "... limit reached" strings (concurrency, context, budget, subagent spawn, fast mode), and a false mark parks a healthy lane for hours. An unparsed or implausible reset time falls back to `DefaultCooldown` (1h) rather than a long park, since being wrong-short self-corrects on the next failure. -- `no-mistakes doctor` is the read surface: an installed runner with a live mark reports `quota-exhausted until