Skip to content

fix(hooks): soften guard for stale repositories - #3730

Open
Ha1baraA11 wants to merge 1 commit into
Graphify-Labs:v8from
Ha1baraA11:codex/fix-hook-repo-staleness
Open

Ha1baraA11 wants to merge 1 commit into
Graphify-Labs:v8from
Ha1baraA11:codex/fix-hook-repo-staleness

Conversation

@Ha1baraA11

@Ha1baraA11 Ha1baraA11 commented Sep 22, 2026 •

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes #3718 by treating a repository as stale when its latest Git commit is newer than graphify-out/graph.json, even if the specific file being read has an unchanged mtime. The hook now emits the existing non-blocking stale guidance for commits that add or delete nearby modules, while failing open when Git is unavailable or the directory is not a repository.

Type of change

  • Bug fix
  • New feature
  • Documentation
  • Tests or CI
  • Refactor
  • Security fix

How was this tested?

This was tested with the following commands:

uv run --extra openai pytest tests/ -q --tb=short
5736 passed, 96 skipped, 2 warnings

uv run --extra openai pytest tests/test_hook_guard.py tests/test_hook_out_of_project_paths.py tests/test_hook_guard_token_match.py tests/test_hook_strict.py -q
147 passed, 4 skipped

uv run --extra openai ruff check graphify/cli.py tests/test_hook_strict.py
uv run --extra openai python -m tools.skillgen --check
uv run --extra openai python -m tools.skillgen --audit-coverage
graphify update .
git diff --check

The regression test simulates a fresh target file with a newer repository commit and confirms strict mode softens to the stale guidance instead of denying the read.

Graphify-specific checklist

  • I added or updated tests for behavior changes.
  • I updated documentation or confirmed that no documentation is needed.
  • I updated generated skill artifacts when changing their source fragments.
  • I considered compatibility across supported Python versions.
  • I confirmed that no API keys, generated graph data, or local-only files are included.

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


Graphify review — findings

Extends the strict read-hook staleness check to treat a repository as stale when its latest git commit is newer than the graph build, catching cases where nearby modules were added or removed even though the read file's own mtime is unchanged. The git lookup is optional and fails open — outside a repo or on any subprocess/parse error the check is skipped rather than denying — and a stale repo softens the hook to a nudge, never a hard deny. Also updates the stale-read nudge text to mention repository-level staleness alongside per-file changes.

Worth a look

  • git commit mtime compared to graph mtime uses wall-clock vs filesystem mtime, and commit time need not exceed build time even when repo changed — graphify/cli.py:934 · 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 — 281 functions depend on the 79 functions this change touches.

Health — this change adds coupling hotspots:

  • new: dispatch_command() — 2 callers, 125 callees
  • new: _stale_graph_sources() — 7 callers, 6 callees
  • new: _run_hook_guard() — 4 callers, 8 callees
  • new: test_poisoned_manifest_is_healed() — 0 callers, 6 callees

Verification — 281 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: 219 function(s) in the blast radius were not formally verified this run

Test selection

Test selection

24 of 290 test file(s) selected (8%) via static blast radius.

  • tests/test_affected_cli.py — impact
  • tests/test_agents_platform.py — impact
  • tests/test_codebuddy.py — impact
  • tests/test_devin.py — impact
  • tests/test_explain_cli.py — impact
  • tests/test_extract_cli.py — impact
  • tests/test_global_add_tag_inference.py — impact
  • tests/test_god_nodes_cli.py — impact
  • tests/test_hollow_chunks_arm_shrink_guard.py — impact
  • tests/test_hook_guard_token_match.py — impact
  • tests/test_hook_out_of_project_paths.py — impact
  • tests/test_hook_strict.py — impact, changed-test
  • tests/test_incomplete_build_guard.py — impact
  • tests/test_install.py — impact
  • tests/test_install_references.py — impact
  • tests/test_merge_chunks_validation.py — impact
  • tests/test_multigraph_diagnostics.py — impact
  • tests/test_no_dedup_flag.py — impact
  • tests/test_partial_cache.py — impact
  • tests/test_path_cli.py — impact
  • tests/test_query_cli.py — impact
  • tests/test_query_induced_edges.py — impact
  • tests/test_stale_prune.py — impact
  • tests/test_unverified_semantic_shrink.py — impact

Selection is safe under the controlled-regression assumption; always-run tests + a periodic full run are the backstops. Advisory — it never changes the check verdict.

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

@safishamsi

Copy link
Copy Markdown
Collaborator

Thanks @Ha1baraA11 — the intent is good (a per-file-fresh but repo-stale graph should soften to the non-blocking nudge, never hard-deny) and the fail-open is correct. Two things to address before merge:

  1. Per-read git log subprocess cost. As written the git probe runs on essentially every strict Read of a fresh in-project file. A git spawn is 1s+ on AV-scanned Windows (the codebase avoids exactly this elsewhere), and a PreToolUse hook runs synchronously before the agent's read — so this is a real latency regression. Please memoize the repo-HEAD time per session (there's session-marker infra already) or only run the probe when a deny is otherwise imminent.
  2. Comparison semantics. graph.json st_mtime (sub-second, build-write wall clock) vs git %ct (second-granularity committer time, rebase-mutable) is a best-effort heuristic that can both over- and under-flag. It's tolerable because the outcome is only a nudge, but please add a comment saying so, and add fail-open + "older-commit-does-not-soften" regression tests. Appreciate it.

This branch has not been deployed

No deployments
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.

hook-guard: repo-level staleness keeps emitting the MANDATORY nudge after recent commits

2 participants