Skip to content

perf: derive deobfuscate paths in shell - #185

Merged
junior-ricon merged 4 commits into
mainfrom
junior/deobfuscate-path-builtins
Jul 28, 2026
Merged

perf: derive deobfuscate paths in shell#185
junior-ricon merged 4 commits into
mainfrom
junior/deobfuscate-path-builtins

Conversation

@junior-ricon

Copy link
Copy Markdown
Contributor

Summary

  • derive scoped deobfuscation IDs with Bash parameter expansion instead of basename
  • derive readable target directories in shell instead of one dirname process per restored note
  • keep dirty-readable checks, partial-success state, suppression, staging, and move behavior unchanged
  • add focused guards proving the target paths do not invoke those external commands

Validation

  • mise run test test/deobfuscate.bats (29 passed)
  • mise run test test/rename.bats (25 passed)
  • mise run test (414 BATS and 9 Python tests passed)
  • codebase lint . (8 rules passed)
  • mise run doctor
  • bash -n .mise/tasks/deobfuscate lib/obfuscate.sh
  • git diff --check
  • Fold git:pre-commit and git:pre-push (no failures)

@johnson-ricon johnson-ricon left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Change request

.mise/tasks/deobfuscate:31 changes basename semantics for scoped paths ending in /. For notes/<id>/, ${arg##*/} produces an empty ID. Because full_mode then remains true, notes deobfuscate "notes/<id>/" restores every manifest entry instead of only the requested note. I reproduced this on exact head 8c91b5f902943a893b06bd1f48968a77d2478b06; the regression failed because all three fixture notes were restored.

Fix-it: #186. It trims trailing slashes with Bash 3.2-compatible parameter expansion before deriving the final component and adds a regression that proves the other notes remain obfuscated.

Review coverage

I independently reviewed the full diff and stressed top-level and nested restores, Bash 3.2-compatible expansion, dirty-readable refusal, clean refresh, mid-batch partial failure/state recording, scoped/full suppression, and no-staging behavior. The new basename guard does catch the exact former scoped subprocess call, and the dirname guard blocks every dirname invocation during the nested rename_to_readable path; existing behavior tests cover top-level and nested restoration. Those guards prove subprocess removal for the intended old calls, but the basename guard does not prove semantic equivalence, which is where the trailing-slash regression escaped.

Hosted Ubuntu and macOS checks are green on the reviewed head. On the fix-it branch I ran the focused deobfuscate suite (30 passed), full suite (415 BATS and 9 Python passed), all 8 convention lints, doctor, README generation check, Bash syntax checks, and git diff --check. The optional clone-local pre-commit hook is not installed. I found no change to mv, locking, timing, broader deobfuscation behavior, suppression mechanics, or staging beyond the scoped-path issue above.

@johnson-ricon johnson-ricon left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Re-reviewed exact head aade93244282686aa8f663485f557fa8a619253d; the earlier trailing-slash change request is resolved.

The new slash-only fail-safe preserves basename semantics for all-slash inputs without allowing an empty scoped ID to select full mode. The regression proves notes deobfuscate "////" leaves both fixture notes obfuscated, while the existing trailing-slash case still restores only the requested note. I also rechecked the shell-only basename/dirname replacements, top-level and nested restore behavior, scoped suppression, dirty-readable protection, partial-failure state recording, and no-staging behavior. I found no remaining correctness issue in the diff.

Local validation on this exact head: focused deobfuscate/rename suite, 56 passed; Bash syntax and git diff --check, passed. Hosted Ubuntu and macOS checks are green.

@junior-ricon
junior-ricon merged commit d993b76 into main Jul 28, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants