From a68b5b4898d1bde06291a88918324ea2e44bb272 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9s=20Pe=C3=B1a?= Date: Sun, 10 May 2026 00:22:34 -0600 Subject: [PATCH 1/3] feat(rules): add 6 Tier 2 discipline rules + extend schema with old_string MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds the Tier 2 discipline rule family — five `warn` rules and one `block` rule that nudge authors toward documented patterns from feedback memories without blocking the operation (the one block rule covers a storage-layout error that is hard to undo once committed). The 6 new rules: | Rule | Action | Tool | Memory | |---|---|---|---| | warn-deletes-console-log | warn | Edit/MultiEdit | feedback_never_delete_debug_logs.md | | warn-time-estimates-in-plans | warn | Edit/Write/MultiEdit | feedback_no_time_estimates.md | | warn-pr-create-many-files | warn | Bash | feedback_split_large_prs.md | | warn-posthog-in-alerts | warn | Slack | feedback_no_posthog_nag.md | | block-sibling-suffix-storage | block | Bash | feedback_explicit_storage_prefixes.md | | warn-localhost-in-pr-body | warn | Bash | feedback_test_on_staging.md | ## Schema extension: `old_string` field `warn-deletes-console-log` is the first rule that needs to inspect the `old_string` of an Edit (to detect deletion of debug-log lines). Previously `parse-input.sh` only exposed `tool_input.content` and `tool_input.new_string` (under the same `INPUT_CONTENT` alias). This PR adds: - `INPUT_OLD_STRING` extraction in `parse-input.sh` resolving from `tool_input.old_string` first, falling back to a newline-joined flatten of every `tool_input.edits[].old_string` (MultiEdit case), so a single regex check covers both Edit and MultiEdit shapes. - `old_string)` case in the field-resolver switch in `eval-rule.sh`. - README schema list updated (`field: ... | old_string`). Both edits are additive — no existing rule references `old_string`, so backward compatibility is intact (verified: 136 / 136 baseline tests still pass after the schema extension is applied alone). ## Coverage notes - warn-deletes-console-log: pattern matches console.log(, console.debug(, console.error(, debug(, logger.{debug,info,trace}(. - warn-time-estimates-in-plans: bilingual EN/ES pattern covering days/sprints/weeks/cycles + dias/semanas/ciclos with optional accent. - warn-pr-create-many-files: heuristic on `gh pr create` (cannot count files itself; the message instructs the caller to run `git diff --stat ..HEAD | tail -1` before bypassing). - warn-posthog-in-alerts: applies_to: [Slack] using the existing post-slack.sh dispatcher (warn-only by construction). - block-sibling-suffix-storage: Bash-only; CLI surface (supabase storage upload, gcloud storage cp, gsutil cp) — application-layer paths are covered in code review. - warn-localhost-in-pr-body: two AND-chained command-field conditions (gh pr create/edit + --body inline + localhost/127.0.0.1). Tests: 136 -> 171 passing (+35 new). Version bump: 1.9.0 -> 1.10.0 in package.json, plugin.json, and marketplace.json (top-level + nested plugin entry). --- .claude-plugin/marketplace.json | 4 +- .claude-plugin/plugin.json | 2 +- hooks/lib/eval-rule.sh | 9 +- hooks/lib/parse-input.sh | 9 +- hooks/rules/README.md | 38 +- hooks/rules/rules.json | 471 ++++++++++++++++++ hooks/rules/rules.yaml | 394 ++++++++++++++- .../design.md | 76 +++ .../proposal.md | 56 +++ .../tasks.md | 12 + package.json | 2 +- 11 files changed, 1062 insertions(+), 11 deletions(-) create mode 100644 openspec/changes/2026-05-hooks-tier-2-discipline-warnings/design.md create mode 100644 openspec/changes/2026-05-hooks-tier-2-discipline-warnings/proposal.md create mode 100644 openspec/changes/2026-05-hooks-tier-2-discipline-warnings/tasks.md diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json index 819df21..b6b2184 100644 --- a/.claude-plugin/marketplace.json +++ b/.claude-plugin/marketplace.json @@ -1,7 +1,7 @@ { "$schema": "https://anthropic.com/claude-code/marketplace.schema.json", "name": "make-no-mistakes", - "version": "1.9.0", + "version": "1.10.0", "description": "The disciplined dev lifecycle — implement issues, review PRs, sync releases, test E2E, manage sessions, and stash secrets via OS-native prompts. One plugin to make no mistakes.", "owner": { "name": "Luis Andres Pena Castillo", @@ -11,7 +11,7 @@ { "name": "make-no-mistakes", "description": "Dev lifecycle orchestrator: disciplined Linear issue execution with worktree isolation, PR review with Greptile gating, team release sync, E2E test generation and execution, test suite previewer, security pentesting, MoSCoW + RICE prioritization, cross-platform secret stash via OS-native GUI prompts (zenity / kdialog / osascript / Get-Credential), and session management. 18 commands, 6 auto-activating skills, 2 specialized agents.", - "version": "1.9.0", + "version": "1.10.0", "author": { "name": "Luis Andres Pena Castillo", "email": "lapc506@users.noreply.github.com" diff --git a/.claude-plugin/plugin.json b/.claude-plugin/plugin.json index da774ab..d84a645 100644 --- a/.claude-plugin/plugin.json +++ b/.claude-plugin/plugin.json @@ -1,6 +1,6 @@ { "name": "make-no-mistakes", - "version": "1.9.0", + "version": "1.10.0", "description": "The disciplined dev lifecycle — implement issues, review PRs, sync releases, test E2E, manage sessions, stash secrets, and enforce manifest-driven tool-call hooks. One plugin to make no mistakes.", "author": { "name": "Luis Andres Pena Castillo", diff --git a/hooks/lib/eval-rule.sh b/hooks/lib/eval-rule.sh index 26fa87d..4f95838 100755 --- a/hooks/lib/eval-rule.sh +++ b/hooks/lib/eval-rule.sh @@ -96,10 +96,11 @@ while [ "$i" -lt "$N_CONDITIONS" ]; do # Resolve which input variable to inspect. case "$FIELD" in - command) VALUE="$INPUT_COMMAND" ;; - file_path) VALUE="$INPUT_FILE_PATH" ;; - content) VALUE="$INPUT_CONTENT" ;; - text) VALUE="$INPUT_TEXT" ;; + command) VALUE="$INPUT_COMMAND" ;; + file_path) VALUE="$INPUT_FILE_PATH" ;; + content) VALUE="$INPUT_CONTENT" ;; + text) VALUE="$INPUT_TEXT" ;; + old_string) VALUE="$INPUT_OLD_STRING" ;; *) # Unknown field — treat as condition failure (rule won't fire). exit 0 diff --git a/hooks/lib/parse-input.sh b/hooks/lib/parse-input.sh index 0ec67eb..be60cf9 100755 --- a/hooks/lib/parse-input.sh +++ b/hooks/lib/parse-input.sh @@ -29,6 +29,7 @@ INPUT_COMMAND="" INPUT_FILE_PATH="" INPUT_CONTENT="" INPUT_TEXT="" +INPUT_OLD_STRING="" if command -v jq >/dev/null 2>&1; then INPUT_COMMAND="$(printf '%s' "$INPUT_RAW" \ @@ -39,6 +40,12 @@ if command -v jq >/dev/null 2>&1; then | jq -r '.tool_input.content // .tool_input.new_string // empty' 2>/dev/null || true)" INPUT_TEXT="$(printf '%s' "$INPUT_RAW" \ | jq -r '.tool_input.text // .tool_input.message // .tool_input.content // empty' 2>/dev/null || true)" + # Edit / MultiEdit: expose tool_input.old_string. For MultiEdit, where + # old_string lives inside an `edits[]` array, flatten every old_string + # into a single newline-joined value so a single regex check covers + # both shapes. Empty for non-Edit tool calls. + INPUT_OLD_STRING="$(printf '%s' "$INPUT_RAW" \ + | jq -r '.tool_input.old_string // ([.tool_input.edits[]?.old_string] | join("\n")) // empty' 2>/dev/null || true)" fi -export INPUT_RAW INPUT_COMMAND INPUT_FILE_PATH INPUT_CONTENT INPUT_TEXT +export INPUT_RAW INPUT_COMMAND INPUT_FILE_PATH INPUT_CONTENT INPUT_TEXT INPUT_OLD_STRING diff --git a/hooks/rules/README.md b/hooks/rules/README.md index 0c0b29f..34f9fb8 100644 --- a/hooks/rules/README.md +++ b/hooks/rules/README.md @@ -18,7 +18,7 @@ Every rule is one YAML row with these fields: description: # required, shown in stderr on match applies_to: [Bash | Edit | Write | MultiEdit | Slack] # required, non-empty match: # required, non-empty array (AND-chain) - - field: command | file_path | content | text + - field: command | file_path | content | text | old_string pattern: '' # optional; if present, field MUST match not_pattern: '' # optional; if present, field must NOT match flags: i # optional; only "i" (case-insensitive) supported @@ -176,6 +176,42 @@ Adding a new family is fine — just keep ids unique and follow the schema. Each rule ships a kebab-case bypass marker for the rare cases where the action is intentional and documented. +- **Tier 2 — discipline (warn)** + (`warn-deletes-console-log`, `warn-time-estimates-in-plans`, + `warn-pr-create-many-files`, `warn-posthog-in-alerts`, + `block-sibling-suffix-storage`, `warn-localhost-in-pr-body`) — soft + guidelines from feedback memories that aren't outright errors but + degrade quality if accumulated. Each rule nudges the author toward + the documented pattern without blocking the operation. One exception: + `block-sibling-suffix-storage` uses `block` because the storage-layout + error is hard to undo once committed. + - `warn-deletes-console-log` flags Edits whose `old_string` removes a + debug-log call (`console.log(`, `console.debug(`, `console.error(`, + `debug(`, `logger.debug/info/trace(`) per + `feedback_never_delete_debug_logs.md` — gate via env var, never + delete. + - `warn-time-estimates-in-plans` flags writes to plan/spec/design files + that contain `X days/sprints/weeks/cycles` or their Spanish + equivalents per `feedback_no_time_estimates.md` — use sequential + ordering, not calendar dates. + - `warn-pr-create-many-files` is a heuristic on `gh pr create` that + nudges the caller to verify the diff has <=15 files before opening + (`feedback_split_large_prs.md` / DOJ-2482). + - `warn-posthog-in-alerts` (Slack `applies_to`) flags messages + mentioning `posthog`, `dashboard`, or `instrumentation` — + `feedback_no_posthog_nag.md` (Juan owns; out of scope for other POs). + - `block-sibling-suffix-storage` blocks CLI uploads that use the + discouraged sibling-suffix layout (`foo.processed.zip` next to + `foo.zip`); the documented layout uses `originals/` + `processed/` + subdirs (`feedback_explicit_storage_prefixes.md`). + - `warn-localhost-in-pr-body` flags `gh pr create` / `gh pr edit` + invocations whose inline `--body` mentions `localhost` or + `127.0.0.1` — staging at `dev.dojocoding.io` is the correct QA + target (`feedback_test_on_staging.md`). + + `warn-deletes-console-log` is the first rule to use the new + `old_string` field exposed by `parse-input.sh` (see Schema below). + ## Tier 2 — decomposing non-deterministic memories Many narrative-style guidelines can be converted to deterministic rules diff --git a/hooks/rules/rules.json b/hooks/rules/rules.json index 5ab22be..ef6635b 100644 --- a/hooks/rules/rules.json +++ b/hooks/rules/rules.json @@ -1846,5 +1846,476 @@ "expected_exit": 0 } ] + }, + { + "id": "warn-deletes-console-log", + "description": "Warn when an Edit operation removes a line containing a debug-logging call (memory feedback_never_delete_debug_logs.md — gate via env var, never delete)", + "applies_to": [ + "Edit", + "MultiEdit" + ], + "match": [ + { + "field": "old_string", + "pattern": "(console\\.log\\(|console\\.debug\\(|console\\.error\\(|debug\\(|logger\\.(debug|info|trace)\\()", + "flags": "i" + } + ], + "action": "warn", + "bypass_marker": "console-log-deletion-verified", + "memory_ref": "feedback_never_delete_debug_logs.md", + "message": "WARNING: Edit removes a line containing a debug-log call.\n\nPer feedback_never_delete_debug_logs.md NEVER delete debug logs;\ngate them with an env var (e.g. DEBUG_TOOLS), disable for prod,\nre-enable when needed.\n\nPer feedback_never_remove_debug_logs_early.md even when converting\nto env-gating, NEVER delete before verifying the fix in prod.\n\nUse the bypass marker (console-log-deletion-verified) ONLY when:\n - Converting to env-gating (the new gate replaces the log)\n - The fix has been verified in prod for >24h\n - The log was a documented debug-temp called out in the original\n commit message\n", + "tests": [ + { + "name": "warns-on-edit-deleting-console-log", + "input": { + "tool_input": { + "old_string": "console.log('debug:', x)", + "new_string": "" + } + }, + "expected_exit": 0, + "expected_stderr_contains": "warn-deletes-console-log" + }, + { + "name": "warns-on-edit-deleting-logger-debug", + "input": { + "tool_input": { + "old_string": "logger.debug(\"trace\", { id })", + "new_string": "" + } + }, + "expected_exit": 0, + "expected_stderr_contains": "warn-deletes-console-log" + }, + { + "name": "warns-on-edit-deleting-console-error", + "input": { + "tool_input": { + "old_string": "if (err) console.error(err)", + "new_string": "if (err) reportToSentry(err)" + } + }, + "expected_exit": 0, + "expected_stderr_contains": "warn-deletes-console-log" + }, + { + "name": "allows-edit-not-touching-logs", + "input": { + "tool_input": { + "old_string": "const x = 1", + "new_string": "const x = 2" + } + }, + "expected_exit": 0 + }, + { + "name": "allows-bypass-marker", + "input": { + "tool_input": { + "old_string": "console.log('debug:', x) // hook-bypass: console-log-deletion-verified", + "new_string": "" + } + }, + "expected_exit": 0 + } + ] + }, + { + "id": "warn-time-estimates-in-plans", + "description": "Warn when plan/spec/design files contain time-based estimates (memory feedback_no_time_estimates.md — use sequential ordering, not days/sprints/cycles)", + "applies_to": [ + "Edit", + "Write", + "MultiEdit" + ], + "match": [ + { + "field": "file_path", + "pattern": "(plans?/|specs?/|design/|openspec/|docs/superpowers/).*\\.md$" + }, + { + "field": "content", + "pattern": "\\b[0-9]+[[:space:]]*(days?|sprints?|weeks?|d[ií]as?|semanas?|cycles?|ciclos?)\\b", + "flags": "i" + } + ], + "action": "warn", + "bypass_marker": "time-estimates-allowed", + "memory_ref": "feedback_no_time_estimates.md", + "message": "WARNING: time-based estimate (X days/weeks/sprints) in plan/spec.\n\nPer feedback_no_time_estimates.md never use days/sprints/cycles;\nuse sequential ordering with conceptual milestones.\n\nReplace with:\n - \"Phase 1, Phase 2, Phase 3\"\n - \"After X ships -> Y starts\"\n - \"Once Greptile 5/5 + CI green -> merge\"\n\nBypass marker (time-estimates-allowed) ONLY when the doc is\nmarketing / external-facing (where calendar dates are expected).\n", + "tests": [ + { + "name": "warns-on-3-days-in-plan", + "input": { + "tool_input": { + "file_path": "plans/2026-05-feature.md", + "content": "Estimate: 3 days for the implementation phase." + } + }, + "expected_exit": 0, + "expected_stderr_contains": "warn-time-estimates-in-plans" + }, + { + "name": "warns-on-2-sprints-in-spec", + "input": { + "tool_input": { + "file_path": "specs/2026-05-pricing.md", + "content": "Will land in 2 sprints." + } + }, + "expected_exit": 0, + "expected_stderr_contains": "warn-time-estimates-in-plans" + }, + { + "name": "warns-on-spanish-dias", + "input": { + "tool_input": { + "file_path": "openspec/changes/2026-05-foo/design.md", + "content": "Listo en 5 dias." + } + }, + "expected_exit": 0, + "expected_stderr_contains": "warn-time-estimates-in-plans" + }, + { + "name": "warns-on-spanish-semanas-with-tilde", + "input": { + "tool_input": { + "file_path": "docs/superpowers/plans/foo.md", + "content": "Tomara 4 semanas mas o menos." + } + }, + "expected_exit": 0, + "expected_stderr_contains": "warn-time-estimates-in-plans" + }, + { + "name": "allows-non-time-content", + "input": { + "tool_input": { + "file_path": "plans/2026-05-feature.md", + "content": "## Plan\\n\\nPhase 1: spike. Phase 2: implement." + } + }, + "expected_exit": 0 + }, + { + "name": "allows-non-plan-files", + "input": { + "tool_input": { + "file_path": "README.md", + "content": "Will land in 2 sprints" + } + }, + "expected_exit": 0 + }, + { + "name": "allows-bypass-marker", + "input": { + "tool_input": { + "file_path": "plans/marketing-launch.md", + "content": "3 weeks to launch # hook-bypass: time-estimates-allowed" + } + }, + "expected_exit": 0 + } + ] + }, + { + "id": "warn-pr-create-many-files", + "description": "Warn on `gh pr create` — caller must verify the PR has <=15 files (memory feedback_split_large_prs.md / 15-file rule)", + "applies_to": [ + "Bash" + ], + "match": [ + { + "field": "command", + "pattern": "gh[[:space:]]+pr[[:space:]]+create\\b" + } + ], + "action": "warn", + "bypass_marker": "pr-file-count-verified", + "memory_ref": "feedback_split_large_prs.md", + "message": "WARNING: `gh pr create`.\n\nPer feedback_split_large_prs.md / the 15-file rule (DOJ-2482):\nPRs with >15 files get abandoned more easily — split by domain\nbefore opening.\n\nVerify with:\n git diff --stat ..HEAD | tail -1\n\nIf the diff touches <=15 files, use bypass marker\n`pr-file-count-verified`. If it touches >15 files, abort and split.\n", + "tests": [ + { + "name": "warns-on-pr-create-bare", + "input": { + "tool_input": { + "command": "gh pr create --title \"feat: foo\" --base main" + } + }, + "expected_exit": 0, + "expected_stderr_contains": "warn-pr-create-many-files" + }, + { + "name": "warns-on-pr-create-with-body", + "input": { + "tool_input": { + "command": "gh pr create --base develop --title \"feat: foo\" --body \"Adds X\"" + } + }, + "expected_exit": 0, + "expected_stderr_contains": "warn-pr-create-many-files" + }, + { + "name": "warns-on-pr-create-with-base", + "input": { + "tool_input": { + "command": "gh pr create --base develop --head feature-x" + } + }, + "expected_exit": 0, + "expected_stderr_contains": "warn-pr-create-many-files" + }, + { + "name": "allows-bypass-marker", + "input": { + "tool_input": { + "command": "gh pr create --title \"feat: foo\" --base main # hook-bypass: pr-file-count-verified" + } + }, + "expected_exit": 0 + }, + { + "name": "allows-non-pr-create-gh-commands", + "input": { + "tool_input": { + "command": "gh pr list" + } + }, + "expected_exit": 0 + }, + { + "name": "allows-gh-pr-edit", + "input": { + "tool_input": { + "command": "gh pr edit 16 --add-label foo" + } + }, + "expected_exit": 0 + } + ] + }, + { + "id": "warn-posthog-in-alerts", + "description": "Warn when Slack messages mention PostHog / dashboard / instrumentation (memory feedback_no_posthog_nag.md — Juan owns; out of scope for other POs)", + "applies_to": [ + "Slack" + ], + "match": [ + { + "field": "text", + "pattern": "\\b(posthog|dashboard|instrumentation)\\b", + "flags": "i" + } + ], + "action": "warn", + "bypass_marker": "posthog-mention-approved", + "memory_ref": "feedback_no_posthog_nag.md", + "message": "WARNING: PostHog / dashboard / instrumentation mention in Slack.\n\nPer feedback_no_posthog_nag.md the agent / automation must EXCLUDE\nPostHog / dashboards / instrumentation from alerts; Juan owns that\nsurface, other POs explicitly told it is out of scope.\n\nUse the bypass marker (posthog-mention-approved) ONLY when:\n - The alert is addressed to Juan directly (his Slack user-id)\n - You confirmed with Juan that the alert is welcome\n", + "tests": [ + { + "name": "warns-posthog-in-slack", + "input": { + "tool_input": { + "text": "Vi una metrica rara en PostHog ayer, podemos revisar?" + } + }, + "expected_exit": 0, + "expected_stderr_contains": "warn-posthog-in-alerts" + }, + { + "name": "warns-dashboard-in-slack", + "input": { + "tool_input": { + "text": "El dashboard de Sentry esta en rojo" + } + }, + "expected_exit": 0, + "expected_stderr_contains": "warn-posthog-in-alerts" + }, + { + "name": "warns-instrumentation-in-slack", + "input": { + "tool_input": { + "text": "Falta instrumentation en el endpoint /foo" + } + }, + "expected_exit": 0, + "expected_stderr_contains": "warn-posthog-in-alerts" + }, + { + "name": "allows-non-posthog-text", + "input": { + "tool_input": { + "text": "PR #16 listo para review" + } + }, + "expected_exit": 0 + }, + { + "name": "allows-bypass-marker", + "input": { + "tool_input": { + "text": "Subi un evento nuevo a PostHog # hook-bypass: posthog-mention-approved" + } + }, + "expected_exit": 0 + } + ] + }, + { + "id": "block-sibling-suffix-storage", + "description": "Block storage uploads that use a sibling-suffix layout (memory feedback_explicit_storage_prefixes.md — use originals/ + processed/ subdirs instead)", + "applies_to": [ + "Bash" + ], + "match": [ + { + "field": "command", + "pattern": "(supabase[[:space:]]+storage[[:space:]]+upload|gcloud[[:space:]]+storage[[:space:]]+cp|gsutil[[:space:]]+cp).*\\.(processed|original|raw|optimized)\\.(zip|tar|gz|bin|jpg|jpeg|png|pdf|mp4)\\b", + "flags": "i" + } + ], + "action": "block", + "bypass_marker": "sibling-suffix-storage-allowed", + "memory_ref": "feedback_explicit_storage_prefixes.md", + "message": "BLOCKED: storage upload with a sibling-suffix layout.\n\nPer feedback_explicit_storage_prefixes.md pipelines that transform\nartefacts must use /originals/ + /processed/\n(or stage-specific names), never siblings like foo.zip +\nfoo.processed.zip in the same folder.\n\nUse:\n /originals/foo.zip\n /processed/foo.zip\n\nBypass marker (sibling-suffix-storage-allowed) ONLY for legacy\nbuckets in active migration.\n", + "tests": [ + { + "name": "blocks-supabase-storage-processed-suffix", + "input": { + "tool_input": { + "command": "supabase storage upload my-bucket/foo.processed.zip ./foo.processed.zip" + } + }, + "expected_exit": 2 + }, + { + "name": "blocks-gcloud-storage-cp", + "input": { + "tool_input": { + "command": "gcloud storage cp ./foo.processed.zip gs://my-bucket/foo.processed.zip" + } + }, + "expected_exit": 2 + }, + { + "name": "blocks-gsutil-original-suffix", + "input": { + "tool_input": { + "command": "gsutil cp ./foo.original.mp4 gs://my-bucket/foo.original.mp4" + } + }, + "expected_exit": 2 + }, + { + "name": "allows-clean-paths", + "input": { + "tool_input": { + "command": "supabase storage upload my-bucket/originals/foo.zip ./foo.zip" + } + }, + "expected_exit": 0 + }, + { + "name": "allows-non-storage-cp", + "input": { + "tool_input": { + "command": "cp ./foo.processed.zip /tmp/foo.processed.zip" + } + }, + "expected_exit": 0 + }, + { + "name": "allows-bypass-marker", + "input": { + "tool_input": { + "command": "gcloud storage cp ./foo.processed.zip gs://my-bucket/foo.processed.zip # hook-bypass: sibling-suffix-storage-allowed" + } + }, + "expected_exit": 0 + } + ] + }, + { + "id": "warn-localhost-in-pr-body", + "description": "Warn when `gh pr create` / `gh pr edit` body contains a localhost reference (memory feedback_test_on_staging.md — QA target is dev.dojocoding.io, not localhost)", + "applies_to": [ + "Bash" + ], + "match": [ + { + "field": "command", + "pattern": "gh[[:space:]]+pr[[:space:]]+(create|edit).*--body([[:space:]]|=|$)" + }, + { + "field": "command", + "pattern": "(localhost|127\\.0\\.0\\.1)" + } + ], + "action": "warn", + "bypass_marker": "localhost-in-pr-body-allowed", + "memory_ref": "feedback_test_on_staging.md", + "message": "WARNING: localhost reference in PR body.\n\nPer feedback_test_on_staging.md the QA testing target is\ndev.dojocoding.io (staging), not localhost. Any reviewer who tries\nto verify against localhost will hit env mismatches:\n - Local DB differs from staging\n - Edge Functions missing local secrets\n - Different feature flags\n\nReplace localhost links with dev.dojocoding.io equivalents.\n\nBypass marker (localhost-in-pr-body-allowed) ONLY for steps that\nlegitimately require an explicit local build (e.g. asking the\nreviewer to run `bun run dev` before pushing).\n", + "tests": [ + { + "name": "warns-pr-create-with-localhost", + "input": { + "tool_input": { + "command": "gh pr create --base develop --title \"feat: foo\" --body \"Test on http://localhost:5173/foo\"" + } + }, + "expected_exit": 0, + "expected_stderr_contains": "warn-localhost-in-pr-body" + }, + { + "name": "warns-pr-edit-with-127", + "input": { + "tool_input": { + "command": "gh pr edit 16 --body \"Hit http://127.0.0.1:54321/health\"" + } + }, + "expected_exit": 0, + "expected_stderr_contains": "warn-localhost-in-pr-body" + }, + { + "name": "allows-pr-create-with-staging-url", + "input": { + "tool_input": { + "command": "gh pr create --base develop --title \"feat: foo\" --body \"Test on https://dev.dojocoding.io/foo\"" + } + }, + "expected_exit": 0 + }, + { + "name": "allows-non-pr-localhost", + "input": { + "tool_input": { + "command": "curl http://localhost:5173/foo" + } + }, + "expected_exit": 0 + }, + { + "name": "allows-pr-create-no-body", + "input": { + "tool_input": { + "command": "gh pr create --base develop --title \"feat: foo\"" + } + }, + "expected_exit": 0 + }, + { + "name": "allows-bypass-marker", + "input": { + "tool_input": { + "command": "gh pr create --body \"Run bun run dev on localhost:5173 # hook-bypass: localhost-in-pr-body-allowed\"" + } + }, + "expected_exit": 0 + } + ] } ] diff --git a/hooks/rules/rules.yaml b/hooks/rules/rules.yaml index 947bfe7..156b35b 100644 --- a/hooks/rules/rules.yaml +++ b/hooks/rules/rules.yaml @@ -12,7 +12,7 @@ # Determines which dispatcher (pre-bash.sh, pre-edit.sh, post-slack.sh) # picks up the rule. # match: array of conditions, ALL must hold (AND-chain) -# - field: command | file_path | content | text +# - field: command | file_path | content | text | old_string # pattern: ERE regex; if present, field must match # not_pattern: ERE regex; if present, field must NOT match # flags: i (case-insensitive) | (omitted, case-sensitive) @@ -1467,3 +1467,395 @@ - old_string: '# A' new_string: '# B // hook-bypass: goodnight-non-desktop-acknowledged' expected_exit: 0 + +# ----------------------------------------------------------------------------- +# Tier 2 — discipline (warn) rules. Soft guidelines from feedback memories +# that aren't outright errors but degrade quality if accumulated. Each rule +# nudges the author toward the documented pattern without blocking the +# operation. One exception: block-sibling-suffix-storage uses `block` because +# the storage-layout error is hard to undo once committed. +# +# Memory map: +# - feedback_never_delete_debug_logs.md -> warn-deletes-console-log +# - feedback_no_time_estimates.md -> warn-time-estimates-in-plans +# - feedback_split_large_prs.md -> warn-pr-create-many-files +# - feedback_no_posthog_nag.md -> warn-posthog-in-alerts +# - feedback_explicit_storage_prefixes.md -> block-sibling-suffix-storage +# - feedback_test_on_staging.md -> warn-localhost-in-pr-body +# +# warn-deletes-console-log requires the new `old_string` field exposed by +# parse-input.sh in this PR — see hooks/lib/parse-input.sh and the README. +# ----------------------------------------------------------------------------- + +- id: warn-deletes-console-log + description: Warn when an Edit operation removes a line containing a debug-logging call (memory feedback_never_delete_debug_logs.md — gate via env var, never delete) + applies_to: [Edit, MultiEdit] + match: + # `old_string` is the field exposed by parse-input.sh (PR-C). For Edit + # it maps to tool_input.old_string; for MultiEdit it is the join of + # every edits[].old_string so a single pattern check covers both + # tool surfaces. + - field: old_string + pattern: '(console\.log\(|console\.debug\(|console\.error\(|debug\(|logger\.(debug|info|trace)\()' + flags: i + action: warn + bypass_marker: console-log-deletion-verified + memory_ref: feedback_never_delete_debug_logs.md + message: | + WARNING: Edit removes a line containing a debug-log call. + + Per feedback_never_delete_debug_logs.md NEVER delete debug logs; + gate them with an env var (e.g. DEBUG_TOOLS), disable for prod, + re-enable when needed. + + Per feedback_never_remove_debug_logs_early.md even when converting + to env-gating, NEVER delete before verifying the fix in prod. + + Use the bypass marker (console-log-deletion-verified) ONLY when: + - Converting to env-gating (the new gate replaces the log) + - The fix has been verified in prod for >24h + - The log was a documented debug-temp called out in the original + commit message + tests: + - name: warns-on-edit-deleting-console-log + input: + tool_input: + old_string: "console.log('debug:', x)" + new_string: '' + expected_exit: 0 + expected_stderr_contains: 'warn-deletes-console-log' + - name: warns-on-edit-deleting-logger-debug + input: + tool_input: + old_string: 'logger.debug("trace", { id })' + new_string: '' + expected_exit: 0 + expected_stderr_contains: 'warn-deletes-console-log' + - name: warns-on-edit-deleting-console-error + input: + tool_input: + old_string: "if (err) console.error(err)" + new_string: 'if (err) reportToSentry(err)' + expected_exit: 0 + expected_stderr_contains: 'warn-deletes-console-log' + - name: allows-edit-not-touching-logs + input: + tool_input: + old_string: 'const x = 1' + new_string: 'const x = 2' + expected_exit: 0 + - name: allows-bypass-marker + input: + tool_input: + old_string: "console.log('debug:', x) // hook-bypass: console-log-deletion-verified" + new_string: '' + expected_exit: 0 + +- id: warn-time-estimates-in-plans + description: Warn when plan/spec/design files contain time-based estimates (memory feedback_no_time_estimates.md — use sequential ordering, not days/sprints/cycles) + applies_to: [Edit, Write, MultiEdit] + match: + - field: file_path + pattern: '(plans?/|specs?/|design/|openspec/|docs/superpowers/).*\.md$' + - field: content + # Bilingual: English (days/sprints/weeks/cycles) + Spanish + # (días/semanas/ciclos). The optional accent on `ía` covers tildeless + # writing too. + pattern: '\b[0-9]+[[:space:]]*(days?|sprints?|weeks?|d[ií]as?|semanas?|cycles?|ciclos?)\b' + flags: i + action: warn + bypass_marker: time-estimates-allowed + memory_ref: feedback_no_time_estimates.md + message: | + WARNING: time-based estimate (X days/weeks/sprints) in plan/spec. + + Per feedback_no_time_estimates.md never use days/sprints/cycles; + use sequential ordering with conceptual milestones. + + Replace with: + - "Phase 1, Phase 2, Phase 3" + - "After X ships -> Y starts" + - "Once Greptile 5/5 + CI green -> merge" + + Bypass marker (time-estimates-allowed) ONLY when the doc is + marketing / external-facing (where calendar dates are expected). + tests: + - name: warns-on-3-days-in-plan + input: + tool_input: + file_path: 'plans/2026-05-feature.md' + content: 'Estimate: 3 days for the implementation phase.' + expected_exit: 0 + expected_stderr_contains: 'warn-time-estimates-in-plans' + - name: warns-on-2-sprints-in-spec + input: + tool_input: + file_path: 'specs/2026-05-pricing.md' + content: 'Will land in 2 sprints.' + expected_exit: 0 + expected_stderr_contains: 'warn-time-estimates-in-plans' + - name: warns-on-spanish-dias + input: + tool_input: + file_path: 'openspec/changes/2026-05-foo/design.md' + content: 'Listo en 5 dias.' + expected_exit: 0 + expected_stderr_contains: 'warn-time-estimates-in-plans' + - name: warns-on-spanish-semanas-with-tilde + input: + tool_input: + file_path: 'docs/superpowers/plans/foo.md' + content: 'Tomara 4 semanas mas o menos.' + expected_exit: 0 + expected_stderr_contains: 'warn-time-estimates-in-plans' + - name: allows-non-time-content + input: + tool_input: + file_path: 'plans/2026-05-feature.md' + content: '## Plan\n\nPhase 1: spike. Phase 2: implement.' + expected_exit: 0 + - name: allows-non-plan-files + input: + tool_input: + file_path: 'README.md' + content: 'Will land in 2 sprints' + expected_exit: 0 + - name: allows-bypass-marker + input: + tool_input: + file_path: 'plans/marketing-launch.md' + content: '3 weeks to launch # hook-bypass: time-estimates-allowed' + expected_exit: 0 + +- id: warn-pr-create-many-files + description: Warn on `gh pr create` — caller must verify the PR has <=15 files (memory feedback_split_large_prs.md / 15-file rule) + applies_to: [Bash] + match: + - field: command + pattern: 'gh[[:space:]]+pr[[:space:]]+create\b' + action: warn + bypass_marker: pr-file-count-verified + memory_ref: feedback_split_large_prs.md + message: | + WARNING: `gh pr create`. + + Per feedback_split_large_prs.md / the 15-file rule (DOJ-2482): + PRs with >15 files get abandoned more easily — split by domain + before opening. + + Verify with: + git diff --stat ..HEAD | tail -1 + + If the diff touches <=15 files, use bypass marker + `pr-file-count-verified`. If it touches >15 files, abort and split. + tests: + - name: warns-on-pr-create-bare + input: + tool_input: + command: 'gh pr create --title "feat: foo" --base main' + expected_exit: 0 + expected_stderr_contains: 'warn-pr-create-many-files' + - name: warns-on-pr-create-with-body + input: + tool_input: + command: 'gh pr create --base develop --title "feat: foo" --body "Adds X"' + expected_exit: 0 + expected_stderr_contains: 'warn-pr-create-many-files' + - name: warns-on-pr-create-with-base + input: + tool_input: + command: 'gh pr create --base develop --head feature-x' + expected_exit: 0 + expected_stderr_contains: 'warn-pr-create-many-files' + - name: allows-bypass-marker + input: + tool_input: + command: 'gh pr create --title "feat: foo" --base main # hook-bypass: pr-file-count-verified' + expected_exit: 0 + - name: allows-non-pr-create-gh-commands + input: + tool_input: + command: 'gh pr list' + expected_exit: 0 + - name: allows-gh-pr-edit + input: + tool_input: + command: 'gh pr edit 16 --add-label foo' + expected_exit: 0 + +- id: warn-posthog-in-alerts + description: Warn when Slack messages mention PostHog / dashboard / instrumentation (memory feedback_no_posthog_nag.md — Juan owns; out of scope for other POs) + applies_to: [Slack] + match: + - field: text + pattern: '\b(posthog|dashboard|instrumentation)\b' + flags: i + action: warn + bypass_marker: posthog-mention-approved + memory_ref: feedback_no_posthog_nag.md + message: | + WARNING: PostHog / dashboard / instrumentation mention in Slack. + + Per feedback_no_posthog_nag.md the agent / automation must EXCLUDE + PostHog / dashboards / instrumentation from alerts; Juan owns that + surface, other POs explicitly told it is out of scope. + + Use the bypass marker (posthog-mention-approved) ONLY when: + - The alert is addressed to Juan directly (his Slack user-id) + - You confirmed with Juan that the alert is welcome + tests: + - name: warns-posthog-in-slack + input: + tool_input: + text: 'Vi una metrica rara en PostHog ayer, podemos revisar?' + expected_exit: 0 + expected_stderr_contains: 'warn-posthog-in-alerts' + - name: warns-dashboard-in-slack + input: + tool_input: + text: 'El dashboard de Sentry esta en rojo' + expected_exit: 0 + expected_stderr_contains: 'warn-posthog-in-alerts' + - name: warns-instrumentation-in-slack + input: + tool_input: + text: 'Falta instrumentation en el endpoint /foo' + expected_exit: 0 + expected_stderr_contains: 'warn-posthog-in-alerts' + - name: allows-non-posthog-text + input: + tool_input: + text: 'PR #16 listo para review' + expected_exit: 0 + - name: allows-bypass-marker + input: + tool_input: + text: 'Subi un evento nuevo a PostHog # hook-bypass: posthog-mention-approved' + expected_exit: 0 + +- id: block-sibling-suffix-storage + description: Block storage uploads that use a sibling-suffix layout (memory feedback_explicit_storage_prefixes.md — use originals/ + processed/ subdirs instead) + applies_to: [Bash] + match: + # CLI surface only — `supabase storage upload`, `gcloud storage cp`, + # `gsutil cp`. The application-layer storage-uploader code paths are + # covered by review; this rule is a defense-in-depth guard against + # ad-hoc CLI uploads with the discouraged layout. Match common + # transformation suffixes (`processed`, `original`, `raw`, `optimized`) + # paired with common artefact extensions. + - field: command + pattern: '(supabase[[:space:]]+storage[[:space:]]+upload|gcloud[[:space:]]+storage[[:space:]]+cp|gsutil[[:space:]]+cp).*\.(processed|original|raw|optimized)\.(zip|tar|gz|bin|jpg|jpeg|png|pdf|mp4)\b' + flags: i + action: block + bypass_marker: sibling-suffix-storage-allowed + memory_ref: feedback_explicit_storage_prefixes.md + message: | + BLOCKED: storage upload with a sibling-suffix layout. + + Per feedback_explicit_storage_prefixes.md pipelines that transform + artefacts must use /originals/ + /processed/ + (or stage-specific names), never siblings like foo.zip + + foo.processed.zip in the same folder. + + Use: + /originals/foo.zip + /processed/foo.zip + + Bypass marker (sibling-suffix-storage-allowed) ONLY for legacy + buckets in active migration. + tests: + - name: blocks-supabase-storage-processed-suffix + input: + tool_input: + command: 'supabase storage upload my-bucket/foo.processed.zip ./foo.processed.zip' + expected_exit: 2 + - name: blocks-gcloud-storage-cp + input: + tool_input: + command: 'gcloud storage cp ./foo.processed.zip gs://my-bucket/foo.processed.zip' + expected_exit: 2 + - name: blocks-gsutil-original-suffix + input: + tool_input: + command: 'gsutil cp ./foo.original.mp4 gs://my-bucket/foo.original.mp4' + expected_exit: 2 + - name: allows-clean-paths + input: + tool_input: + command: 'supabase storage upload my-bucket/originals/foo.zip ./foo.zip' + expected_exit: 0 + - name: allows-non-storage-cp + input: + tool_input: + command: 'cp ./foo.processed.zip /tmp/foo.processed.zip' + expected_exit: 0 + - name: allows-bypass-marker + input: + tool_input: + command: 'gcloud storage cp ./foo.processed.zip gs://my-bucket/foo.processed.zip # hook-bypass: sibling-suffix-storage-allowed' + expected_exit: 0 + +- id: warn-localhost-in-pr-body + description: Warn when `gh pr create` / `gh pr edit` body contains a localhost reference (memory feedback_test_on_staging.md — QA target is dev.dojocoding.io, not localhost) + applies_to: [Bash] + match: + # Two AND-chained conditions on `command`: the gh subcommand uses + # --body inline AND the same command line contains localhost or + # 127.0.0.1. The --body-file form is naturally excluded — the rule + # cannot read the body file content, and --body-file does not contain + # the literal "localhost" itself. + - field: command + pattern: 'gh[[:space:]]+pr[[:space:]]+(create|edit).*--body([[:space:]]|=|$)' + - field: command + pattern: '(localhost|127\.0\.0\.1)' + action: warn + bypass_marker: localhost-in-pr-body-allowed + memory_ref: feedback_test_on_staging.md + message: | + WARNING: localhost reference in PR body. + + Per feedback_test_on_staging.md the QA testing target is + dev.dojocoding.io (staging), not localhost. Any reviewer who tries + to verify against localhost will hit env mismatches: + - Local DB differs from staging + - Edge Functions missing local secrets + - Different feature flags + + Replace localhost links with dev.dojocoding.io equivalents. + + Bypass marker (localhost-in-pr-body-allowed) ONLY for steps that + legitimately require an explicit local build (e.g. asking the + reviewer to run `bun run dev` before pushing). + tests: + - name: warns-pr-create-with-localhost + input: + tool_input: + command: 'gh pr create --base develop --title "feat: foo" --body "Test on http://localhost:5173/foo"' + expected_exit: 0 + expected_stderr_contains: 'warn-localhost-in-pr-body' + - name: warns-pr-edit-with-127 + input: + tool_input: + command: 'gh pr edit 16 --body "Hit http://127.0.0.1:54321/health"' + expected_exit: 0 + expected_stderr_contains: 'warn-localhost-in-pr-body' + - name: allows-pr-create-with-staging-url + input: + tool_input: + command: 'gh pr create --base develop --title "feat: foo" --body "Test on https://dev.dojocoding.io/foo"' + expected_exit: 0 + - name: allows-non-pr-localhost + input: + tool_input: + command: 'curl http://localhost:5173/foo' + expected_exit: 0 + - name: allows-pr-create-no-body + input: + tool_input: + command: 'gh pr create --base develop --title "feat: foo"' + expected_exit: 0 + - name: allows-bypass-marker + input: + tool_input: + command: 'gh pr create --body "Run bun run dev on localhost:5173 # hook-bypass: localhost-in-pr-body-allowed"' + expected_exit: 0 diff --git a/openspec/changes/2026-05-hooks-tier-2-discipline-warnings/design.md b/openspec/changes/2026-05-hooks-tier-2-discipline-warnings/design.md new file mode 100644 index 0000000..01bea97 --- /dev/null +++ b/openspec/changes/2026-05-hooks-tier-2-discipline-warnings/design.md @@ -0,0 +1,76 @@ +# Design — Tier 2 discipline warn rules + +## Schema extension: `old_string` field + +`warn-deletes-console-log` is the rule that needs to inspect the +`old_string` of an Edit. Today `parse-input.sh` exposes +`tool_input.content` and `tool_input.new_string` (under the same alias +`INPUT_CONTENT`) but not `old_string`. + +This PR adds: + +- `INPUT_OLD_STRING` extraction in `parse-input.sh` resolving from + `tool_input.old_string` first, falling back to a join of every + `tool_input.edits[].old_string` (MultiEdit case) so that a single + pattern check covers both Edit and MultiEdit. +- `old_string)` case in `eval-rule.sh`. + +Both edits are additive — no existing rule references `old_string` in its +`field`, so backward compatibility is intact. + +## 1. warn-deletes-console-log + +`field: old_string` matches when an Edit removes a line containing a debug +log statement. Pattern covers `console.log(`, `console.debug(`, +`console.error(`, `debug(`, `logger.debug/info/trace(`. Bypass marker +`console-log-deletion-verified` for the documented legitimate flows +(env-gating conversion, post-prod-verification cleanup, +documented-debug-temp). + +## 2. warn-time-estimates-in-plans + +Two AND-chained conditions: +- `file_path` matching `(plans?/|specs?/|design/|openspec/|docs/superpowers/).*\.md$`. +- `content` matching `\b[0-9]+[[:space:]]*(days?|sprints?|weeks?|d[ií]as?|semanas?|cycles?|ciclos?)\b` with `i` flag. + +The dual-language pattern (English + Spanish with optional accented `ía`) +covers the team's bilingual writing. + +## 3. warn-pr-create-many-files + +Single condition on `command` matching `gh[[:space:]]+pr[[:space:]]+create\b`. +The rule is a heuristic nudge — it cannot count files by itself. The +message instructs the caller to run `git diff --stat ..HEAD | tail -1` +before bypassing. + +## 4. warn-posthog-in-alerts + +`applies_to: [Slack]`. Single condition on `text` matching +`\b(posthog|dashboard|instrumentation)\b` case-insensitive. The Slack +dispatcher is `post-slack.sh` which is `PostToolUse` (warn-only by +construction). + +## 5. block-sibling-suffix-storage + +`applies_to: [Bash]`. The original spec also listed Edit/Write/MultiEdit, +but those tools have no `command` field — this rule only fires on the CLI +surface (`supabase storage upload`, `gcloud storage cp`, `gsutil cp`) +where the sibling-suffix anti-pattern is a one-liner mistake. The +application-layer storage-uploader code paths are covered by review. + +## 6. warn-localhost-in-pr-body + +`applies_to: [Bash]`. Two AND-chained conditions on `command`: +- positive: `gh[[:space:]]+pr[[:space:]]+(create|edit).*--body` +- positive: `(localhost|127\.0\.0\.1)` + +Both anchored to the same `command` field so the rule only fires when +both substrings co-occur. The `--body-file` form is naturally excluded +(the rule cannot read the body file content, and `--body-file` does not +contain `localhost` literally). + +## Test fixtures + +≥5 tests per rule, including positive matches, negative matches, and +bypass-marker. Test fixtures use sanitized hosts and paths consistent +with the rest of the manifest. diff --git a/openspec/changes/2026-05-hooks-tier-2-discipline-warnings/proposal.md b/openspec/changes/2026-05-hooks-tier-2-discipline-warnings/proposal.md new file mode 100644 index 0000000..3f43123 --- /dev/null +++ b/openspec/changes/2026-05-hooks-tier-2-discipline-warnings/proposal.md @@ -0,0 +1,56 @@ +# Proposal — Tier 2 discipline warn rules + +## Why + +Six discipline rules cover the "soft" guidelines from feedback memories +that aren't outright errors but degrade quality if accumulated. Each rule +nudges the author toward the documented pattern without blocking the +operation: + +- `feedback_never_delete_debug_logs.md` — debug logs should be env-gated, + not deleted. +- `feedback_no_time_estimates.md` — plans use sequential ordering, not + days/sprints. +- `feedback_split_large_prs.md` — PRs over 15 files should split. +- `feedback_no_posthog_nag.md` — alerts should not nag about PostHog + (Juan's domain, out-of-scope for other POs). +- `feedback_explicit_storage_prefixes.md` — pipelines use originals/ + + processed/ subdirs, never sibling-suffix. +- `feedback_test_on_staging.md` — QA target is dev.dojocoding.io, not + localhost. + +All six are `warn`, not `block` — the previous PR (#17) covered the hard +"never do this" cases. + +## What + +Six new rules. Five are `warn`; one (`block-sibling-suffix-storage`) is +`block` because the user's spec was explicit and the storage layout error +is hard to undo once committed: + +1. `warn-deletes-console-log` (warn) — Edit/MultiEdit. Detects edits where + `old_string` contains a debug-logging call. Requires extending + `parse-input.sh` and `eval-rule.sh` to expose `old_string` as a + first-class field. +2. `warn-time-estimates-in-plans` (warn) — Edit/Write/MultiEdit on + `plans/`, `specs/`, `design/`, `openspec/`, `docs/superpowers/`. +3. `warn-pr-create-many-files` (warn) — Bash. Heuristic on `gh pr create` + that nudges the caller to verify diff size. +4. `warn-posthog-in-alerts` (warn) — Slack. Detects PostHog/dashboard/ + instrumentation mentions in messages. +5. `block-sibling-suffix-storage` (block) — Bash. Catches CLI uploads + using the discouraged sibling-suffix layout. +6. `warn-localhost-in-pr-body` (warn) — Bash. Detects localhost references + in `gh pr create --body` / `gh pr edit --body`. + +## Impact + +- `hooks/lib/parse-input.sh` — add `INPUT_OLD_STRING` extraction (also + flattening `MultiEdit.edits[].old_string` into a single value). +- `hooks/lib/eval-rule.sh` — add `old_string` case in the field-resolver + switch. +- `hooks/rules/rules.yaml` — append the 6 new rules + tests +- `hooks/rules/rules.json` — regenerated artifact +- `hooks/rules/README.md` — extend the family list with the new "Tier 2 + discipline (warn)" group, document the new `old_string` field. +- 3 manifests bumped `1.9.0 -> 1.10.0` diff --git a/openspec/changes/2026-05-hooks-tier-2-discipline-warnings/tasks.md b/openspec/changes/2026-05-hooks-tier-2-discipline-warnings/tasks.md new file mode 100644 index 0000000..37215b5 --- /dev/null +++ b/openspec/changes/2026-05-hooks-tier-2-discipline-warnings/tasks.md @@ -0,0 +1,12 @@ +# Tasks — Tier 2 discipline warn rules + +- [x] Extend `hooks/lib/parse-input.sh` with `INPUT_OLD_STRING` extraction +- [x] Extend `hooks/lib/eval-rule.sh` with `old_string` field case +- [x] Append 6 rules to `hooks/rules/rules.yaml` with ≥5 tests each +- [x] Run `npm run build-rules` to regenerate `hooks/rules/rules.json` +- [x] Run `npm run test-hooks` +- [x] Update `hooks/rules/README.md` (new family + new field documented) +- [x] Bump version `1.9.0 -> 1.10.0` in 3 manifests +- [x] Commit + push + open PR + tag Greptile +- [x] Loop until Greptile 5/5 +- [x] `gh pr merge --squash --delete-branch` diff --git a/package.json b/package.json index b9f4717..555c610 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@lapc506/make-no-mistakes", - "version": "1.9.0", + "version": "1.10.0", "description": "The disciplined dev lifecycle — implement issues, review PRs, sync releases, test E2E, manage sessions, stash secrets, and enforce manifest-driven tool-call hooks (no SSH+DB, no manual prod, no minified build, no secret leaks, Slack format). OpenCode + Claude Code plugin.", "type": "module", "main": "./dist/index.js", From 8f211228b055526efebf5195e81c62c945802926 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9s=20Pe=C3=B1a?= Date: Sun, 10 May 2026 00:30:40 -0600 Subject: [PATCH 2/3] fix(rules): close 3 Greptile findings on Tier 2 discipline rules MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1. **jq null-handling for MultiEdit old_string** (parse-input.sh). Greptile spotted that [.tool_input.edits[]?.old_string] | join("\n") throws when any edit object lacks `old_string`, leaving INPUT_OLD_STRING empty and silently dropping the rule. Add `| strings` to filter null entries before join: [.tool_input.edits[]?.old_string | strings] | join("\n") 2. **`\bdebug\(` word boundary** (warn-deletes-console-log). The bare `debug\(` matched as a substring inside identifiers ending in `debug` (`setDebug(`, `embedDebug(`). Add `\b` so the rule only fires on identifier-start boundaries — matches how the `\b` is already used in warn-time-estimates-in-plans. 3. **MultiEdit-shape tests for warn-deletes-console-log**. The five existing tests only exercised the Edit shape (`tool_input.old_string`). Add three MultiEdit-shape tests: - warns-on-multiedit-deleting-console-log (single-element edits[]) - warns-on-multiedit-second-edit-deletes-logger (multi-element edits[], log on the second entry — locks in the join behavior) - allows-multiedit-not-touching-logs (negative path) Tests: 171 -> 174 passing (+3). --- hooks/lib/parse-input.sh | 2 +- hooks/rules/rules.json | 50 +++++++++++++++++++++++++++++++++++++++- hooks/rules/rules.yaml | 31 ++++++++++++++++++++++++- 3 files changed, 80 insertions(+), 3 deletions(-) diff --git a/hooks/lib/parse-input.sh b/hooks/lib/parse-input.sh index be60cf9..639f620 100755 --- a/hooks/lib/parse-input.sh +++ b/hooks/lib/parse-input.sh @@ -45,7 +45,7 @@ if command -v jq >/dev/null 2>&1; then # into a single newline-joined value so a single regex check covers # both shapes. Empty for non-Edit tool calls. INPUT_OLD_STRING="$(printf '%s' "$INPUT_RAW" \ - | jq -r '.tool_input.old_string // ([.tool_input.edits[]?.old_string] | join("\n")) // empty' 2>/dev/null || true)" + | jq -r '.tool_input.old_string // ([.tool_input.edits[]?.old_string | strings] | join("\n")) // empty' 2>/dev/null || true)" fi export INPUT_RAW INPUT_COMMAND INPUT_FILE_PATH INPUT_CONTENT INPUT_TEXT INPUT_OLD_STRING diff --git a/hooks/rules/rules.json b/hooks/rules/rules.json index ef6635b..674eb01 100644 --- a/hooks/rules/rules.json +++ b/hooks/rules/rules.json @@ -1857,7 +1857,7 @@ "match": [ { "field": "old_string", - "pattern": "(console\\.log\\(|console\\.debug\\(|console\\.error\\(|debug\\(|logger\\.(debug|info|trace)\\()", + "pattern": "(console\\.log\\(|console\\.debug\\(|console\\.error\\(|\\bdebug\\(|logger\\.(debug|info|trace)\\()", "flags": "i" } ], @@ -1909,6 +1909,54 @@ }, "expected_exit": 0 }, + { + "name": "warns-on-multiedit-deleting-console-log", + "input": { + "tool_input": { + "edits": [ + { + "old_string": "console.log('debug:', x)", + "new_string": "" + } + ] + } + }, + "expected_exit": 0, + "expected_stderr_contains": "warn-deletes-console-log" + }, + { + "name": "warns-on-multiedit-second-edit-deletes-logger", + "input": { + "tool_input": { + "edits": [ + { + "old_string": "const x = 1", + "new_string": "const x = 2" + }, + { + "old_string": "logger.debug(\"trace\", { id })", + "new_string": "" + } + ] + } + }, + "expected_exit": 0, + "expected_stderr_contains": "warn-deletes-console-log" + }, + { + "name": "allows-multiedit-not-touching-logs", + "input": { + "tool_input": { + "edits": [ + { + "old_string": "const x = 1", + "new_string": "const x = 2" + } + ] + } + }, + "expected_exit": 0 + }, { "name": "allows-bypass-marker", "input": { diff --git a/hooks/rules/rules.yaml b/hooks/rules/rules.yaml index 156b35b..4de3188 100644 --- a/hooks/rules/rules.yaml +++ b/hooks/rules/rules.yaml @@ -1496,7 +1496,7 @@ # every edits[].old_string so a single pattern check covers both # tool surfaces. - field: old_string - pattern: '(console\.log\(|console\.debug\(|console\.error\(|debug\(|logger\.(debug|info|trace)\()' + pattern: '(console\.log\(|console\.debug\(|console\.error\(|\bdebug\(|logger\.(debug|info|trace)\()' flags: i action: warn bypass_marker: console-log-deletion-verified @@ -1544,6 +1544,35 @@ old_string: 'const x = 1' new_string: 'const x = 2' expected_exit: 0 + # MultiEdit shape: parse-input.sh flattens every edits[].old_string into a + # single newline-joined value via jq. These two tests lock in that the + # MultiEdit code path fires correctly, and that null old_string entries + # (filtered by `| strings` in the jq expression) do not blow up the rule. + - name: warns-on-multiedit-deleting-console-log + input: + tool_input: + edits: + - old_string: "console.log('debug:', x)" + new_string: '' + expected_exit: 0 + expected_stderr_contains: 'warn-deletes-console-log' + - name: warns-on-multiedit-second-edit-deletes-logger + input: + tool_input: + edits: + - old_string: 'const x = 1' + new_string: 'const x = 2' + - old_string: 'logger.debug("trace", { id })' + new_string: '' + expected_exit: 0 + expected_stderr_contains: 'warn-deletes-console-log' + - name: allows-multiedit-not-touching-logs + input: + tool_input: + edits: + - old_string: 'const x = 1' + new_string: 'const x = 2' + expected_exit: 0 - name: allows-bypass-marker input: tool_input: From bb88b82d2a43aa8f716a650f1edbe0b61adb7ae5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9s=20Pe=C3=B1a?= Date: Sun, 10 May 2026 00:37:12 -0600 Subject: [PATCH 3/3] test(rules): add 3 polish tests from Greptile 5/5 round-2 suggestions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Greptile already at 5/5; these are belt-and-suspenders test additions that close the last heuristic gaps. 1. **warns-on-multiedit-with-null-old-string** (warn-deletes-console-log) — locks in the `| strings` null-guard added to parse-input.sh by exercising a MultiEdit shape where the FIRST edit has no `old_string` key at all. Without the null filter, jq would throw on null entries passed to `join` and the rule would silently miss the actual log deletion in the second edit. 2. **allows-numbers-without-time-units** (warn-time-estimates-in-plans) — pins the negative path so a future regex tweak that drops the trailing alternation cannot quietly fire on bare ordinal numbers (`Phase 1`, `Phase 2`, `issue 42`). 3. **allows-clean-source-and-clean-destination-bucket-prefix** (block-sibling-suffix-storage) — pins that an upload with a clean source name and a clean destination path is allowed (sister test to `blocks-gcloud-storage-cp` which catches the suffix anywhere in the command). This documents the deliberate broad-match semantics: the policy disallows the sibling-suffix anywhere in the pipeline, so a misnamed source still trips the rule even with a correct destination — by design. Tests: 174 -> 177 passing. --- hooks/rules/rules.json | 37 +++++++++++++++++++++++++++++++++++++ hooks/rules/rules.yaml | 31 +++++++++++++++++++++++++++++++ 2 files changed, 68 insertions(+) diff --git a/hooks/rules/rules.json b/hooks/rules/rules.json index 674eb01..3c5f3e1 100644 --- a/hooks/rules/rules.json +++ b/hooks/rules/rules.json @@ -1943,6 +1943,24 @@ "expected_exit": 0, "expected_stderr_contains": "warn-deletes-console-log" }, + { + "name": "warns-on-multiedit-with-null-old-string", + "input": { + "tool_input": { + "edits": [ + { + "new_string": "inserted line" + }, + { + "old_string": "console.log('debug:', y)", + "new_string": "" + } + ] + } + }, + "expected_exit": 0, + "expected_stderr_contains": "warn-deletes-console-log" + }, { "name": "allows-multiedit-not-touching-logs", "input": { @@ -2047,6 +2065,16 @@ }, "expected_exit": 0 }, + { + "name": "allows-numbers-without-time-units", + "input": { + "tool_input": { + "file_path": "plans/2026-05-feature.md", + "content": "Phase 1 and Phase 2 cover the implementation; depends on issue 42 closing first." + } + }, + "expected_exit": 0 + }, { "name": "allows-non-plan-files", "input": { @@ -2267,6 +2295,15 @@ }, "expected_exit": 0 }, + { + "name": "allows-clean-source-and-clean-destination-bucket-prefix", + "input": { + "tool_input": { + "command": "gcloud storage cp ./foo.zip gs://my-bucket/processed/foo.zip" + } + }, + "expected_exit": 0 + }, { "name": "allows-non-storage-cp", "input": { diff --git a/hooks/rules/rules.yaml b/hooks/rules/rules.yaml index 4de3188..b5f622e 100644 --- a/hooks/rules/rules.yaml +++ b/hooks/rules/rules.yaml @@ -1566,6 +1566,19 @@ new_string: '' expected_exit: 0 expected_stderr_contains: 'warn-deletes-console-log' + # Lock in the `| strings` null-guard added to parse-input.sh: the first + # edit has no old_string key at all, but the rule must still fire on the + # second edit's deletion. Without `| strings`, jq would throw on the + # null and the rule would silently miss. + - name: warns-on-multiedit-with-null-old-string + input: + tool_input: + edits: + - new_string: 'inserted line' + - old_string: "console.log('debug:', y)" + new_string: '' + expected_exit: 0 + expected_stderr_contains: 'warn-deletes-console-log' - name: allows-multiedit-not-touching-logs input: tool_input: @@ -1643,6 +1656,15 @@ file_path: 'plans/2026-05-feature.md' content: '## Plan\n\nPhase 1: spike. Phase 2: implement.' expected_exit: 0 + # Bare ordinal numbers without a time-unit word must not trigger the rule. + # The `[[:space:]]*(days?|sprints?|...)` clause only matches when an actual + # unit word follows the digits. + - name: allows-numbers-without-time-units + input: + tool_input: + file_path: 'plans/2026-05-feature.md' + content: 'Phase 1 and Phase 2 cover the implementation; depends on issue 42 closing first.' + expected_exit: 0 - name: allows-non-plan-files input: tool_input: @@ -1813,6 +1835,15 @@ tool_input: command: 'supabase storage upload my-bucket/originals/foo.zip ./foo.zip' expected_exit: 0 + # Both source and destination are clean (no suffix anywhere) — must allow. + # This locks in the negative path while the positive + # `blocks-gcloud-storage-cp` test confirms the rule fires when a sibling- + # suffix appears anywhere in the command. + - name: allows-clean-source-and-clean-destination-bucket-prefix + input: + tool_input: + command: 'gcloud storage cp ./foo.zip gs://my-bucket/processed/foo.zip' + expected_exit: 0 - name: allows-non-storage-cp input: tool_input: