Skip to content

fix(hooks): refresh managed sections safely - #2427

Open
rafaelMep wants to merge 3 commits into
Graphify-Labs:v8from
rafaelMep:fix/update-installed-hooks
Open

fix(hooks): refresh managed sections safely#2427
rafaelMep wants to merge 3 commits into
Graphify-Labs:v8from
rafaelMep:fix/update-installed-hooks

Conversation

@rafaelMep

Copy link
Copy Markdown

Summary

  • refresh existing Graphify-managed hook sections instead of appending duplicate blocks
  • preserve third-party hook content before and after managed sections
  • run managed hook bodies in a subshell so an internal exit 0 cannot suppress later hook content
  • reject malformed Graphify marker layouts without modifying the hook file

Testing

  • focused hook regression tests: 7 passed
  • Ruff and Python compilation checks passed

@graphify-labs graphify-labs Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Looks safe to merge — no coupling regressions and no blocking issues, checked against the code graph (not a self-assessment).

Graphify reviewed this change.

Looks safe to merge — no coupling regressions and no blocking issues, checked against the code graph (not a self-assessment).


Graphify review — findings

This PR modifies Graphify's git hook installation logic in graphify/hooks.py. The _install_hook function is reworked to take an explicit end marker and now updates an existing managed hook section in place (replacing content between start/end markers), raises an error on malformed or duplicated markers, and returns an "updated existing" message when a stale section is refreshed. The generated post-commit and post-checkout shell scripts are wrapped in a subshell ( ... ), and the install() caller and tests are updated accordingly, with new tests covering stale-section updates, malformed-marker rejection, and hook execution behavior. The surface area is the hook install/update path, the embedded shell script templates, and the corresponding test suite (including a new helper for locating a POSIX shell).

No blocking issues surfaced. 5 lower-confidence candidates did not survive cross-model review.

Analysis details — impact, health, verification

Impact & health

Graphify review

Impact — 457 functions depend on the 324 functions this change touches.

Health — this change adds coupling hotspots:

  • worse: install() — 28 callers, 6 callees

Verification — 457 functions in the blast radius were not formally verified this run (proofs are advisory here).

Gate & verification

graphify gate

PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.

Advisory (not blocking):

  • verification_scope: 330 function(s) in the blast radius were not formally verified this run

· 1 more finding(s) on lines outside this diff (see the check run).

@rafaelMep
rafaelMep force-pushed the fix/update-installed-hooks branch from 6075e42 to c1ebe3a Compare August 27, 2026 12:18

@graphify-labs graphify-labs Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Graphify reviewed this change.

Worth a look — the grounded gate found no coupling regressions or blocking issues, but 1 advisory finding(s) below merit a look before merge.

Formal verification. No changes could be formally verified in this run.


Graphify review — findings

Wraps each generated hook body in a ( … ) subshell so an early exit inside Graphify's section no longer aborts unrelated hook content appended after it. Rewrites _install_hook to read/write with fixed \n newlines, require the start marker to appear exactly once before a matching end marker, and raise RuntimeError("malformed Graphify section …") without touching the file when the markers are missing, duplicated, or out of order; updates now also re-chmod the hook to 0o755.

Worth a look

  • install() can write one hook before rejecting a malformed managed section in anothergraphify/hooks.py:776 · Escalate · medium
    • agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
Analysis details — impact, health, verification

Impact & health

Graphify review

Impact — 546 functions depend on the 389 functions this change touches.

Health — this change adds coupling hotspots:

  • new: install() — 35 callers, 7 callees
  • new: dispatch_command() — 2 callers, 122 callees
  • new: dispatch_install_cli() — 2 callers, 31 callees
  • new: status() — 8 callers, 6 callees
  • new: uninstall() — 9 callers, 5 callees
  • new: uninstall_all() — 2 callers, 13 callees
  • new: test_poisoned_manifest_is_healed() — 0 callers, 6 callees

Verification — 546 functions in the blast radius were not formally verified this run (proofs are advisory here).

Gate & verification

graphify gate

PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.

Advisory (not blocking):

  • verification_scope: 396 function(s) in the blast radius were not formally verified this run

Formal verification

Could not verify: Could not verify install.

The verifier did not have enough to check install, so it is saying so rather than guessing. No false assurance is the whole point.

Guarantee: No guarantee either way, this is an honest abstention, not a pass.

Note: Reason: parameter `path` is annotated `Path` — outside the synthesizable primitive/collection set

Could not verify: Could not verify \_install\_hook.

The verifier did not have enough to check \_install\_hook, so it is saying so rather than guessing. No false assurance is the whole point.

Guarantee: No guarantee either way, this is an honest abstention, not a pass.

Note: Reason: parameter `hooks_dir` is annotated `Path` — outside the synthesizable primitive/collection set

· 7 more finding(s) on lines outside this diff (see the check run).

@graphify-labs graphify-labs Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Graphify reviewed this change.

Worth a look — the grounded gate found no coupling regressions or blocking issues, but 2 advisory finding(s) below merit a look before merge.

Formal verification. No changes could be formally verified in this run.


Graphify review — findings

Wraps each generated hook's managed section in a ( … ) subshell so a Graphify early exit terminates only its own body and no longer suppresses unrelated hook content appended after it. Adds _validated_hook_content, which _install_hook and install call to reject hooks with missing, duplicated, or out-of-order managed-section markers by raising RuntimeError before any write; install validates both post-commit and post-checkout up front so neither is modified when either is malformed. Updated sections are now rewritten with normalized newlines and re-chmodded to 0o755.

Worth a look

  • marker_end positional now required but validated content path uses index without end presence guaranteegraphify/hooks.py:555 · Escalate · high
    • agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
  • install() now raises on previously tolerated malformed managed markersgraphify/hooks.py:795 · Escalate · medium
    • agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
Analysis details — impact, health, verification

Impact & health

Graphify review

Impact — 549 functions depend on the 392 functions this change touches.

Health — this change adds coupling hotspots:

  • new: install() — 36 callers, 8 callees
  • new: dispatch_command() — 2 callers, 122 callees
  • new: dispatch_install_cli() — 2 callers, 31 callees
  • new: status() — 8 callers, 6 callees
  • new: uninstall() — 9 callers, 5 callees
  • new: uninstall_all() — 2 callers, 13 callees
  • new: test_poisoned_manifest_is_healed() — 0 callers, 6 callees

Verification — 549 functions in the blast radius were not formally verified this run (proofs are advisory here).

Gate & verification

graphify gate

PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.

Advisory (not blocking):

  • verification_scope: 399 function(s) in the blast radius were not formally verified this run

Formal verification

Could not verify: Could not verify install.

The verifier did not have enough to check install, so it is saying so rather than guessing. No false assurance is the whole point.

Guarantee: No guarantee either way, this is an honest abstention, not a pass.

Note: Reason: parameter `path` is annotated `Path` — outside the synthesizable primitive/collection set

Could not verify: Could not verify \_install\_hook.

The verifier did not have enough to check \_install\_hook, so it is saying so rather than guessing. No false assurance is the whole point.

Guarantee: No guarantee either way, this is an honest abstention, not a pass.

Note: Reason: parameter `hooks_dir` is annotated `Path` — outside the synthesizable primitive/collection set

· 7 more finding(s) on lines outside this diff (see the check run).

@rafaelMep

Copy link
Copy Markdown
Author

Thanks for the follow-up review. I inspected both advisories.

Regarding the marker_end finding: _validated_hook_content() guarantees that both markers occur exactly once and in the correct order before returning. The boolean expression short-circuits, so content.index(marker_end) is never evaluated when the end marker is absent. _install_hook() then uses the same validated content, so its index() call has that presence guarantee.

Regarding compatibility: rejecting malformed Graphify markers is intentional. Previously, malformed layouts could be silently treated as already installed, leaving an ambiguous or stale managed section. The new behavior fails before writing and preserves both hook files unchanged.

The atomicity case is covered by test_install_validates_both_hooks_before_writing, and the focused hook suite passes locally: 103 passed, 1 skipped, with 2 unrelated Windows/MSYS tests deselected.

No additional code change appears necessary for these advisories.

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