Skip to content

Remove empty deny block after notify cleanup#3794

Merged
louis030195 merged 6 commits into
screenpipe:mainfrom
isanchez404:fix/notification-toggle-deny-order
Jun 7, 2026
Merged

Remove empty deny block after notify cleanup#3794
louis030195 merged 6 commits into
screenpipe:mainfrom
isanchez404:fix/notification-toggle-deny-order

Conversation

@isanchez404

@isanchez404 isanchez404 commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

Summary

  • remove an empty permissions.deny block after removing Api(POST /notify)
  • preserve sibling allow rules when deny appears before allow in frontmatter
  • add a regression test for the deny-before-allow cleanup case

Validation

  • vet "Fix empty deny-block cleanup when allow follows it in notification toggle frontmatter" --agentic --agent-harness claude --history-loader "python3 ~/.codex/skills/vet/scripts/export_codex_session.py --session-file /Users/isaac.sanchez/.codex/sessions/2026/06/03/rollout-2026-06-03T09-01-53-019e8d93-7a00-7162-9086-ccd5b79846a4.jsonl"
  • node --experimental-strip-types - <<'NODE' ... NODE (asserted deny-before-allow cleanup preserves allow and removes deny:)
  • git diff --check

Risk Notes

  • low risk: only changes empty-block cleanup in notification-toggle.ts
  • does not change notification rule matching or frontmatter insertion logic

Changed Area

  • apps/screenpipe-app-tauri/lib/utils/notification-toggle.ts
  • apps/screenpipe-app-tauri/lib/utils/notification-toggle.test.ts

Skipped Test Rationale

  • bun run typecheck failed in this sandbox with error: bun is unable to write files to tempdir: PermissionDenied
  • bunx vitest run lib/utils/notification-toggle.test.ts failed in this sandbox with the same Bun tempdir permission error

louis030195
louis030195 reviewed Jun 3, 2026
View reviewed changes

@louis030195 louis030195 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@isanchez404 lgtm, great catch and elegant fix!

generated by the screenpipe pr-review pipe (https://screenpi.pe), not written by a human — reply and tag @louis030195 if it got something wrong.

@louis030195

louis030195 commented Jun 3, 2026

Copy link
Copy Markdown
Collaborator

Nice catch. The old hasChildren check used "next line starts with two spaces", which treats a sibling block (allow:) as a child, so an emptied deny: sitting before allow: survived. Comparing indentation depth is the right call.

Verified it end to end (reproduced on main, then ran the enable-path scenarios):

main:     "permissions:\n  deny:\n  allow:\n    - Bash"   (orphaned empty deny:)
this PR:  "permissions:\n  allow:\n    - Bash"            (clean)

deny-only, multi-rule deny, other-fields, allow-before-deny, and deny-before-allow all pass.

One small edge case, not a blocker: for compact YAML where list items sit at the same indent as their key, the new check is slightly worse than the old one.

permissions:
  deny:
  - Api(POST /search)
  - Api(POST /notify)

Removing the notify rule now also drops deny: and reparents - Api(POST /search) under permissions:. The codebase never emits this style (it always writes 4-space items under a 2-space deny:), so it only affects hand-authored frontmatter, and the notify rule is still removed either way.

If you want to cover both, keep the list-item clause but make it indent-aware (verified against both cases):

const childIndent = lines[nextIdx].match(/^\s*/)?.[0].length ?? 0;
const isListItem  = lines[nextIdx].trim().startsWith("-");
const hasChildren = childIndent > blockIndent || (isListItem && childIndent >= blockIndent);

Either way this is a clean improvement.

louis030195
louis030195 reviewed Jun 3, 2026
View reviewed changes
Comment thread apps/screenpipe-app-tauri/lib/utils/notification-toggle.ts Outdated
Comment on lines +148 to +150
const hasChildren =
nextIdx < lines.length &&
(lines[nextIdx].startsWith(" ") || lines[nextIdx].trim().startsWith("-"));
(lines[nextIdx].match(/^\s*/)?.[0].length ?? 0) > blockIndent;

@louis030195 louis030195 Jun 3, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional, if you want to also handle compact same-indent block sequences (keeps the list-item case the old code covered, but indent-aware so the deny-before-allow fix still holds). Verified against both scenarios.

Suggested change
const hasChildren =
nextIdx < lines.length &&
(lines[nextIdx].startsWith(" ") || lines[nextIdx].trim().startsWith("-"));
(lines[nextIdx].match(/^\s*/)?.[0].length ?? 0) > blockIndent;
const childIndent =
nextIdx < lines.length ? lines[nextIdx].match(/^\s*/)?.[0].length ?? 0 : -1;
const isListItem =
nextIdx < lines.length && lines[nextIdx].trim().startsWith("-");
const hasChildren =
childIndent > blockIndent || (isListItem && childIndent >= blockIndent);

@isanchez404 isanchez404 force-pushed the fix/notification-toggle-deny-order branch from e215e30 to dc3a615 Compare June 3, 2026 17:35
louis030195
louis030195 reviewed Jun 5, 2026
View reviewed changes

@louis030195 louis030195 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@isanchez404 nice yaml parsing fix and solid test coverage. looks good to merge.

generated by the screenpipe pr-review pipe (https://screenpi.pe), not written by a human — reply and tag @louis030195 if it got something wrong.

@louis030195 louis030195 merged commit cbf4d0a into screenpipe:main Jun 7, 2026
13 checks passed
louis030195 pushed a commit that referenced this pull request Jun 7, 2026
@claude
test: edge-case coverage for #3845 / #3854 / #3794 merges
2458baa 
Strengthens the unit suites for the three Tier-1 fixes just merged:

- media-file-path (#3845): malformed percent-escapes, no-extension /
  empty / backtick inputs, case-insensitive ext, Windows forward-slash
  path in text, Unix audio-chunk path with spaces+parens; audio/media
  classifier casing; already-wrapped markdown link, > escaping, Windows
  path in a markdown link.
- context-pruning (#3854): malformed <conversation_history> blocks
  (unterminated, close-before-open) fall back to generic head+tail;
  multiple oversized text blocks clamped while non-text blocks untouched;
  non-string/non-array message content ignored; zero window from
  getContextUsage falls back to default.
- notification-toggle (#3794): notify rule detected across a blank line
  in the deny block; enable keeps permissions+allow children when only
  the notify deny rule existed; deny block retained when a non-notify
  rule remains.

Test-only. Full vitest suite green (486 tests). These functions are pure
modules; the wdio e2e suite mocks pi and drives whole-app UI, so the
deterministic edge-case layer for them is vitest (the context-pruning
suite is already documented as the extension's real e2e coverage).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@louis030195 louis030195 louis030195 left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@isanchez404 @louis030195