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

[sveto]

G-1: evalBash now also checks DANGEROUS_PATHS and DANGEROUS_CONTENT; same for mcp.

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 ;.

Problems (worth discussion):

1. ; in a text field: UPDATE t SET col = 'a;b' WHERE id = 5 -> deny ← should be allowed
2. WHERE in a subquery: UPDATE t SET x=(SELECT y FROM z WHERE z.id=1) -> allowed ← should be deny
3. WHERE in a comment: DELETE FROM users -- WHERE never -> allowed ← should be deny

-- such expressions are hard to catch by a regex. SQL is a language with recursive syntax. ;(

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

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

[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