fix(done): suppress on-modify hooks for recursively completed descendants

[birdmanmandbir]

Summary

• task <parent> done was triggering the on-modify hook for every recursively completed child task, causing N+1 hook invocations (and notification spam) when completing a parent with N children
• Fix: wrap the descendant completion loop in CmdDone.cpp with hooks.enable(false) / hooks.enable(oldHooks) so only the parent's completion fires the hook
• Regression test added to hooks.on-modify.test.py: verifies the hook fires exactly once and both parent and child are completed in the DB

Test plan

• Build passes cleanly (make)
• test_recursive_done_suppresses_child_hooks passes: hook fires exactly once, both tasks marked completed
• Existing TestHooksOnModify tests continue to pass

[birdmanmandbir]

pr-review-lead (round 3):

PR Review: fix(done): suppress on-modify hooks for recursively completed descendants

Critical Issues (1)

[CmdDone.cpp:109-119] Exception-unsafe hook state restoration

The manual save/restore pattern is not exception-safe:

auto oldHooks = Context::getContext().hooks.enable(false);
// ... loop ...
Context::getContext().hooks.enable(oldHooks); // ← never runs if loop throws

TDB2::modify() explicitly throws std::string("task no longer exists") (TDB2.cpp:137), and Task methods have dozens of throw sites. If any exception escapes the loop, hooks._enabled stays false for the rest of the process — silently disabling all subsequent hooks (including onExit) with no indication to the user.

Fix: Use a RAII guard. The enable(bool) -> bool API is already shaped perfectly for one:

struct HooksGuard {
  Hooks& hooks;
  bool saved;
  explicit HooksGuard(Hooks& h, bool val) : hooks(h), saved(h.enable(val)) {}
  ~HooksGuard() { hooks.enable(saved); }
  HooksGuard(const HooksGuard&) = delete;
  HooksGuard& operator=(const HooksGuard&) = delete;
};
// Usage:
HooksGuard hooksGuard(Context::getContext().hooks, false);
auto desc = Context::getContext().tdb2.descendants(task.get("uuid"));
// ... loop — restore is automatic at scope exit, even on throw

---

Important Issues (2)

[hooks.on-modify.test.py:186] UUID extraction has trailing period

parent_uuid = out.strip().split()[-1]

The verbose output format is "Created task <uuid>.\n" — so split()[-1] yields "1234abcd." (with trailing dot). This gets passed into parent_id:{parent_uuid} and {parent_uuid} done, likely causing a malformed attribute or task lookup failure. The rest of the test suite uses export-based UUID lookup (see get_uuid() in tree.test.py).

[CmdDone.cpp:111-118] Partial completion leaves inconsistent state with confusing error

If the loop throws mid-iteration (e.g. on descendant 3 of 5), the first N descendants are already written to storage while the rest are not. The top-level catch in main.cpp prints the raw "task no longer exists" string with no context that the error occurred during auto-completion of a descendant and that the task tree is partially completed. Worth wrapping the loop body in a try/catch to either continue-with-warning (best-effort) or re-throw with context.

---

Suggestions (3)

• Test: add grandchild depth — descendants() returns the full subtree but the test only exercises depth-1. A Parent→Child→Grandchild scenario would cover the recursive case.
• Test: rejecting hook on parent — no test for the case where the hook rejects the parent completion; the invariant "if parent rejected, children must not complete" is untested.
• Test: verify hook fired for parent UUID — assertTriggeredCount(1) confirms count but not which task triggered it. Adding a log content check (logs["calls"][0]["input"]["new"]["uuid"] == parent_uuid) would prevent an inverted regression.

---

Strengths

• The semantic intent is correct: suppressing child hooks is the right behavior — users install hooks for tasks they're explicitly acting on, not for auto-cascaded completions.
• hooks.enable() returning the old value is a good API design that makes the guard pattern trivial to add.
• Test covers the core assertion well (count=1 + both tasks completed in DB).
• Consistent with existing test patterns (hook installation, export verification).

---

Verdict: Needs Work

Fix the RAII issue (critical — real exception-safety bug) and the UUID trailing-period issue (important — likely causes test failures) before merge.

[birdmanmandbir]

coder (round 4):

Triage Update

Fixed

• RAII hook state restoration — replaced manual enable(false) / enable(oldHooks) with a local HooksGuard RAII struct. Destructor calls hooks.enable(saved) unconditionally, so hooks are always restored even if tdb2.modify() throws. Compiles cleanly. commit 3e66fb8
• UUID trailing-period bug in test — dropped the rc.verbose=new-uuid output parsing (which yielded "uuid.") in favour of export-based UUID lookup: self.t.export() + next(t["uuid"] for t in ... if t["description"] == ...). commit 3e66fb8
• Grandchild depth coverage — test now builds a Parent → Child → Grandchild tree; asserts all three are completed in the DB. commit 3e66fb8
• Verify hook fired for parent UUID — added logs["input"]["json"][1]["uuid"] == parent_uuid assertion so an inverted regression (child fires instead of parent) would be caught. commit 3e66fb8

Deferred

• Partial completion inconsistent state / error wrapping — tdb2.modify() could throw mid-loop leaving N of M descendants completed with no context in the error. This is a pre-existing issue in the codebase (the descendant loop predates this PR) and fixing it properly requires deciding between best-effort-with-warning vs re-throw-with-context. Tracked separately; out of scope for this fix.

[birdmanmandbir]

pr-review-lead (round 5):

Re-review: fix(done) suppress on-modify hooks for descendants

Fixed ✅

• RAII hook state restoration — HooksGuard struct defined locally with proper explicit constructor, deleted copy/assign, and destructor that unconditionally restores. Struct is instantiated before the descendants loop so it covers the full scope. Correct and clean.
• UUID trailing-period bug — switched to export-based lookup (next(t["uuid"] for t in ... if t["description"] == ...)). Reliable.
• Grandchild depth coverage — Parent → Child → Grandchild tree, all three verified in +COMPLETED.
• Hook fired for parent UUID — logs["input"]["json"][1]["uuid"] == parent_uuid assertion catches an inverted regression.

Deferred (accepted)

• Partial completion inconsistent state — pre-existing issue predating this PR; reasonable to track separately.

VERDICT: LGTM 🟢

Implementation is correct and exception-safe. Test coverage is thorough for the changed behavior.