enhance dangerous pattern recognition in hooks: rf, force push, sql...

[sveto]

Task 1: dangerous-actions pattern coverage (jira #1646)

G-1: evalBash (and MCP shell fields) run DANGEROUS_BASH + DANGEROUS_CONTENT. Path scanning of free-form shell strings (extractPathCandidates/stripQuotes) was added in G-1 but later dropped (see Task 2 simplification) -- a direct Write/Edit to a key file still triggers the path check.

G-2: rm -r -f and similar commands with flags are now being catched, including situations where the flags are written after the folder name: rm myfolder/ -r -f, rm myfolder -rf etc; also including situations when a flag is inside quotation marks (rm '-rf').

G-3: force push with + prefix being catched, excluding cases where + means something else.

G-4: dangerous SQL expressions are being catched, strictly before the first ;.

G-5: .env is now being catched before word end, like in .env and app.env, but not .envfile or foo.envx. -- reverted in Task 2.

G-6: password-like expressions are now catched and republished as KEY=<redacted> instead of KEY=sk-proj-blablah. -- reverted in Task 2).

Task 2: remove overreach ("guard AI actions, not user data") (jira #1754)

Two corrections from architecture review. Rosetta's job is a safety guard against unsolicited AI actions, not policing user input.

Secrets removed. Rosetta must not touch passwords / user data. G-6 fully reverted, G-5 fully reverted, secret content detectors (inline-aws-key, PEM private key) removed.

What remains from G-1's path set is narrowed to irreversible key/credential files (id_rsa, .aws/credentials, .kube/config, .gnupg, .netrc, .pgpass) and reframed as a non-blocking advise notice -- same class as rm -rf.

Hard-deny removed. All former hard-deny patterns (rm -rf /, mkfs, dd, chmod -R 777, curl | sh) are now soft-deny (reconsider).

Evidence is no longer redacted. The old redact branch is gone, the snippet is simply truncated when shown.

Shell path-scan simplified. extractPathCandidates abd stripQuotes were removed.

Task 3: Bash/Shell (Cursor coverage) (jira #1651)

Cursor runs shell commands via a tool named Shell, not Bash, so the dangerous-actions hook never fired for shell commands in Cursor. Now it does.

Task 4: Remove 'evidence', add pre-defined set of rejection reasons.

Mentioned during a tech call. Explored, fixed. There are now 9 possible reasons: unsafe data manipulation, unsafe schema modification, irreversible file deletion, git history rewrite, destructive device operation, unsafe permission change, remote code execution, unsafe infrastructure operation, credential file overwrite.

[github-actions]

Rosetta Triage Review

Summary: This PR strengthens the dangerous-actions hook with six targeted improvements (G-1 through G-6): broader pattern coverage across DANGEROUS_PATHS and DANGEROUS_CONTENT for bash and MCP calls, robust rm -rf flag-order detection, force-push-by-+-refspec detection, SQL destructive-statement guards bounded by ;, tighter .env basename matching, and secret-value redaction in deny-message evidence.

Findings:

• Missing plugin regeneration: The PR updates src/hooks/src/ (TypeScript source) and src/hooks/dist/src/ (tsc output) but does not update the esbuild bundles in plugins/core-*/hooks/*.js. Per the developer guide, venv/bin/python scripts/pre_commit.py must be run to rebuild bundles and sync them into each plugin — without this, end-users on the current plugin release will not receive the new pattern detection until the next plugin publish cycle.
• git push +main edge case is intentionally left unguarded: The PR body notes this is "handled separately" but no existing or new test demonstrates that bare git push +main (where +main is the repository operand) is caught. The comment in patterns.ts correctly documents this as out-of-scope for the refspec lookahead, but the claim it is "handled separately" should be verified or the wording clarified.
• SQL regex known limitations are well-documented: False-positive (semicolon inside a string literal) and false-negative (WHERE in a subquery or comment) cases are explicitly called out in both inline comments and characterization tests. This is the right trade-off for a reconsider-tier guard.
• Test coverage is excellent: 469 lines of new tests cover positive matches, negative safe-command cases, edge cases (quoted flags, flags after operands), and known-limitation pins. The structure correctly uses beforeAll for shared regex references and includes both unit and integration-level (runHook) coverage.
• Code quality is high: The evalShellString refactor correctly unifies bash and MCP shell-field evaluation. The redactSecrets global-copy pattern (building new RegExp from .source to avoid stateful g-flag leakage) is a careful and correct solution to a subtle JS regex pitfall.
• No breaking changes to the public API: The hook interface is unchanged; the changes only expand the set of flagged patterns. Existing # Rosetta-AI-reviewed markers on reconsider-tier patterns continue to work.

Suggestions:

• Run venv/bin/python scripts/pre_commit.py and commit the regenerated plugin bundles (plugins/core-*/hooks/*.js and standalone equivalents) so the fix ships with the plugin release.
• Either add a test confirming git push +main (single positional token acting as the repository) is caught by a fallback pattern, or update the comment to say "not caught by this guard" rather than "handled separately".
• Consider adding SECRET_VALUE_PATTERNS to DANGEROUS_CONTENT checks for the evalMcpCall path if MCP sql/query fields can carry credentials — currently those fields are checked for SQL destructive patterns but not for embedded secrets.

Automated triage by Rosetta agent

[github-actions]

Rosetta Triage Review

Summary: This PR significantly expands the dangerous-actions hook's detection coverage across six dimensions (G-1 through G-6): unified shell evaluation against all three pattern sets (bash, paths, content), robust rm -rf flag-order detection, git push +refspec force-push detection, expanded SQL destructive-statement coverage (DELETE/UPDATE without WHERE, DROP INDEX/VIEW, ALTER DROP COLUMN), improved .env family matching, and secret-value redaction in deny evidence.

Findings:

• G-1 (evalBash + MCP now check all three sets): New evalShellString() helper correctly unifies DANGEROUS_BASH, DANGEROUS_PATHS, and DANGEROUS_CONTENT checks for both Bash tool and MCP shell fields — good DRY refactor. The extractPathCandidates() function handles redirections, path-separator tokens, and unquoted leading-dot tokens with careful false-positive avoidance for quoted contexts (commit messages).
• G-2 (rm flag-order): Lookahead-based approach (RM_RECURSIVE_LA + RM_FORCE_LA) scans forward from \brm\b for both flags independently, correctly handling rm folder -rf, rm '-rf' /, and combined/separate long forms.
• G-3 (force push via +refspec): GIT_FORCE_REFSPEC_LA pattern correctly requires a prior non-option remote argument before the +refspec token to avoid false positives on git push +main used as a remote name.
• G-4 (SQL DELETE/UPDATE without WHERE): The bounded negative-lookahead approach ((?![^;]*\bwhere\b)) is well-reasoned. Known limitations (false positive on ; inside string values; false negative on WHERE inside subquery or comment) are honestly documented in code comments and pinned by characterization tests — this is the correct engineering call at regex tier.
• G-5 (.env family): Pattern /\.env$|^\.env\..+$/ correctly matches app.env, production.env, and .env.local while excluding .envx, .envfile, and .environment.
• G-6 (secret redaction): redactSecrets() builds separate global-flag regex copies from .source to avoid stateful g flag corruption on shared .test()-based pattern objects — subtle and correct.
• Test coverage: 489 new test lines with positive cases, guard (null) cases, characterization tests for known limitations, and cross-cutting G-6×G-1 regression tests. Coverage is thorough.
• dist/src/ committed: src/hooks/dist/src/hooks/dangerous-actions/evaluate.js and patterns.js are TypeScript compilation artifacts committed alongside source. This is consistent with prior repo practice, but may drift if pre_commit.py is not always run before commit. Worth confirming whether CI regenerates these or relies on committed artifacts.

Suggestions:

• Consider whether a CI step should regenerate and verify dist/src/ matches compiled source to prevent drift, rather than committing the compiled output manually.
• The SQL false-negative cases (WHERE in subquery/comment) are accepted gaps — consider adding a TODO comment referencing a future SQL-lexer-based approach if this gets revisited.
• Minor: stripQuotes removes only leading/trailing quotes but does not handle mixed quote styles (e.g. 'text") — acceptable for the current use case (shell token normalization).

Automated triage by Rosetta agent

[mkuznietsov]

Approved

[isolomatov-gd]

Critical issue identified: original and new code did redacting of secrets => this is breaking, this is not to be address by rosetta.

[github-actions]

Rosetta Triage Review

Summary: This PR improves the dangerous-actions hook with better shell pattern recognition (rm with reordered flags, git force-push via + refspec, SQL injection in shell commands), removes the hard-deny tier in favour of soft-deny across the board, drops overreaching secrets/env detectors, and adds Cursor Shell tool coverage alongside the existing Bash mapping.

Findings:

• Scope mismatch in diff: The PR title and description cover only dangerous-actions improvements (Tasks 1–3), but the diff includes 7 new compiled dist/src/ files for hooks unrelated to this PR (codemap-refresh.js, read-once.js, read-once-reset.js, read-once-shared.js, file-coordination.js, state-ops.js, state-store.js). These appear to be TypeScript compilation artifacts for pre-existing source files now committed to dist/ for the first time. The PR description should mention this to make the diff easier to review.
• Hard-deny removal is a notable security policy change: Former hard-deny patterns (curl | sh, mkfs, dd, chmod -R 777, rm -rf /) are now soft-deny (reconsider), meaning an AI can bypass them by appending the Rosetta-AI-reviewed marker. The stated rationale ("guard AI actions, not user data") is sound, but reviewers should explicitly acknowledge this policy weakening — especially for curl | sh (supply-chain risk).
• SQL regex limitations are well-documented: Known false-positive (embedded ; shortens WHERE-scan window) and false-negative (WHERE inside subquery/comment not detected) cases are pinned by characterization tests — good practice.
• Test coverage is comprehensive: dangerous-actions.test.ts grows by 572 lines covering new patterns, known-limitation cases, and the Cursor Shell adapter. The test structure is clear.
• evalShellString refactoring is clean: Properly abstracts shell evaluation shared between the Bash tool and MCP shell fields, eliminating duplication.
• Cursor Shell fix is minimal and correct: Adding 'Shell' to the bash kind in cursor.ts is a one-line, well-targeted change.

Suggestions:

• Add a note in the PR body explaining that the new dist/src/ files are compiled artifacts of pre-existing TypeScript source (not new features introduced by this PR).
• Consider a brief comment in the PR description or code acknowledging the intentional policy decision to make curl | sh soft-deny, so future maintainers understand the reasoning.
• Verify that scripts/pre_commit.py was run and passes cleanly (as required by ARCHITECTURE.md) before merge — specifically that the new dist files match the compiled output of their TypeScript sources.

Automated triage by Rosetta agent

[isolomatov-gd]

@sveto We need to have a call, you do something really wrong with merging branches. I can see again as if you add read-once hook, while it should not even appear in your case. This PR is broken because of that. You merge somehow incorrect.

[isolomatov-gd]

Also on the call I told we don't need evidence, why is it there? What problem does this solve?

[github-actions]

Rosetta Triage Review

Summary: This PR improves the dangerous-actions hook across three tasks: (1) expands bash pattern coverage for rm -rf flag permutations, force-push refspecs, and destructive SQL variants; (2) reverts overreach by removing hard-deny semantics and credential-content detectors, narrowing the hook's scope to AI action safety rather than user-data policing; (3) fixes a Cursor regression where shell commands were never intercepted because Cursor uses a tool named Shell rather than Bash.

Findings:

• Cursor Shell fix (bug): Critical gap — the dangerous-actions hook never fired for Cursor shell commands before this PR. The adapter normalization and hooks.json.tmpl matcher addition together close the gap correctly.
• Hard-deny removal: curl | sh, mkfs, dd, chmod -R 777 and credential-path patterns are downgraded from unconditional block to reconsider (soft-deny) or advise (non-blocking). This is intentional per the architecture review rationale in Task 2. Reviewers should confirm this policy direction is accepted — a determined AI with the # Rosetta-AI-reviewed marker can now proceed past any pattern, including formerly-hard-denied ones like curl | sh.
• SQL WHERE-detection caveats: The new DELETE/UPDATE without WHERE regexes are well-annotated with known false-positive (embedded ; before WHERE) and false-negative (WHERE in a subquery or comment) scenarios. Pinned by characterization tests. Acceptable tradeoff for a reconsider-tier guard.
• rm -rf flag permutation: The lookahead-based composition (RM_RF_GUARD) correctly handles reversed flags, quoted flags, and post-operand flags. Substantially more robust than the previous single-pattern approach.
• Test coverage: 572 lines added to dangerous-actions.test.ts and 32 lines to adapter.cursor.test.ts. Coverage appears thorough based on the new pattern count and adapter regression test.
• No instruction-path changes: No changes to instructions/r*/**; this is hooks runtime code only.
• Plugin template changes minimal and correct: The one-line change in each hooks.json.tmpl file (adding Shell to Cursor's tool matcher list) is the necessary counterpart to the adapter fix.
• .gitignore addition: src/hooks/dist/src/ added — prevents source-map or intermediate TS outputs from leaking into commits.

Suggestions:

• Consider whether the curl | sh downgrade from hard-deny to reconsider fits your threat model. The # Rosetta-AI-reviewed bypass was designed for intentional, user-sanctioned retries, but a hallucinating agent could add the marker without real review. If supply-chain protection is a priority here, this specific pattern may warrant staying as a stronger control.
• The SQL known-limitation comments in patterns.ts are excellent; mirroring a one-line summary in the test file (next to the characterization tests) would help future contributors understand those as intentional gaps, not test todos.

Automated triage by Rosetta agent

[github-actions]

Rosetta Triage Review

Summary: Enhances the dangerous-actions PreToolUse hook with broader rm -rf flag coverage (any order/position), git force-push detection via +-prefixed refspec, SQL destructive statement detection, and Cursor Shell tool support. Simultaneously removes overreach (secret detection, hard-deny tier, evidence echo) in favor of a clean static-reason taxonomy with soft-deny only.

Findings:

• Behavior-breaking changes present: hard-deny tier is fully removed (all patterns now reconsider/soft-deny), the .env path guard and inline secret detectors (inline-aws-key, inline-private-key) are removed, and the hook message format changed (no evidence echo). Existing deployments will observe different hook behavior after this merge. A breaking-change label is recommended but was not found in the repo's label set.

• Potential gap — git push +<refspec> without explicit remote: GIT_FORCE_REFSPEC_LA requires a non-+-prefixed repository token to precede the + refspec (e.g. git push origin +HEAD:main → caught; git push +HEAD:main with implicit origin → not caught). The inline comment says this case is "handled separately" but no such handler is visible. This is a narrow edge case but the claim in the comment is inaccurate and may confuse future readers.

• No end-to-end Cursor Shell dangerous-actions test: The adapter.cursor.test.ts additions cover IDE detection only. There is no integration test confirming that a Cursor Shell fixture containing rm -rf / is actually denied end-to-end via runHook. The hooks.json.tmpl change (adding Shell to the matcher) is correct, but a test fixture + runHook test would harden the regression guard.

• .gitignore entry for src/hooks/dist/src/: Rationale not explained in the PR body. Presumably generated output; a one-line note in the body would clarify intent.

• SQL patterns in DANGEROUS_BASH: Lines 101-106 of the new patterns.ts place SQL patterns inside DANGEROUS_BASH. This is intentional (catches psql -c "DROP TABLE …" in shell), and the PR description covers it (G-4), but the duplication between DANGEROUS_BASH and DANGEROUS_CONTENT (same SQL regexes appear in both) is worth a comment explaining why both arrays carry them.

• Code quality and tests: Otherwise well-crafted. Regex patterns are thoroughly commented (including known SQL-WHERE-detection limitations), the 577-line test addition covers positive matches, negative matches, edge cases, integration via runHook, and regression guards. The static reason taxonomy (REASON const) is a clean design decision.

Suggestions (optional):

• Fix the comment on GIT_FORCE_REFSPEC_LA to accurately describe the unhandled case rather than implying it is "handled separately."
• Add a runHook integration test with a Cursor Shell fixture and a dangerous command to confirm the matcher change works end-to-end.
• Add a brief note to the DANGEROUS_BASH section comment explaining that SQL regexes are intentionally duplicated between DANGEROUS_BASH and DANGEROUS_CONTENT for shell-command contexts.

Automated triage by Rosetta agent