Skip to content

test(langfuse): structural guard for setup_hook + hook file coexistence#1052

Draft
yasinBursali wants to merge 1 commit intoLight-Heart-Labs:mainfrom
yasinBursali:test/langfuse-manifest-hook-coexists
Draft

test(langfuse): structural guard for setup_hook + hook file coexistence#1052
yasinBursali wants to merge 1 commit intoLight-Heart-Labs:mainfrom
yasinBursali:test/langfuse-manifest-hook-coexists

Conversation

@yasinBursali
Copy link
Copy Markdown
Contributor

DRAFT: must merge AFTER #1040. This PR's tests assert that langfuse's service.setup_hook and hooks/post_install.sh exist. Both are added by upstream PR #1040 (the langfuse postgres uid 70 install fix). Until #1040 merges, these tests fail on bare upstream/main — that's expected. Promote to ready-for-review after git rebase upstream/main post-#1040.

What

Add TestLangfuseManifestHook to dream-server/extensions/services/dashboard-api/tests/test_hooks.py. Two narrow structural-guard assertions that langfuse's manifest declares service.setup_hook = "hooks/post_install.sh" AND that the referenced hook file exists on disk.

Why

The langfuse postgres uid 70 install fix relies on service.setup_hook pointing at hooks/post_install.sh. If either drifts (file renamed, manifest field removed, hook deleted), the host agent's _validate_hook_path returns None, _handle_install silently skips the hook at the if hook_path: guard, and langfuse silently regresses to the broken Linux postgres uid mismatch behaviour with no CI signal. The structural guard catches that.

How

Append a single test class to the existing test_hooks.py (the established home for hook-related tests, sharing its import yaml, pathlib.Path, and class-based conventions):

class TestLangfuseManifestHook:
    def test_langfuse_manifest_declares_post_install_hook(self):
        ext_dir = Path(__file__).resolve().parents[2] / "langfuse"
        manifest = yaml.safe_load((ext_dir / "manifest.yaml").read_text())
        setup_hook = manifest.get("service", {}).get("setup_hook")
        assert setup_hook == "hooks/post_install.sh", (...)

    def test_langfuse_post_install_hook_file_exists(self):
        ext_dir = Path(__file__).resolve().parents[2] / "langfuse"
        hook_path = ext_dir / "hooks" / "post_install.sh"
        assert hook_path.is_file(), (...)

Path expression Path(__file__).resolve().parents[2] / "langfuse" is the corrected form: parents[0] = tests/, parents[1] = dashboard-api/, parents[2] = extensions/services/. (The original issue suggestion Path(__file__).resolve().parent.parent / "dream-server" / ... resolves to a nonexistent path — fixed during implementation.)

Defensive manifest.get("service", {}).get("setup_hook") mirrors the host agent's own style at line 441 — produces a clean AssertionError if the entire service: block is removed in some future regression, instead of a less-actionable KeyError.

Assertion messages reference the langfuse postgres uid 70 install fix conceptually rather than a transient PR number, so the wording remains accurate after rebase regardless of whether that fix's PR number changes.

Testing

  • python3 -m py_compile → PASS
  • ruff → clean (no new warnings, no unused imports)
  • Path math validation: Path(__file__).resolve().parents[2] / "langfuse" resolves to extensions/services/langfuse
  • Bare-branch test: FAILS as expected (manifest field + hook file absent on upstream/main until fix(langfuse): chown postgres/clickhouse data dirs to image uids on Linux #1040 lands)
  • Combined-state test (apply gh pr diff 1040 | git apply locally, run tests, revert): both new tests PASS cleanly; revert leaves worktree pristine
  • Existing test classes (TestResolveHook ×7, TestCheckBashVersion ×3, TestHookEnvAllowlist ×1): 11 / 11 still pass — no regression

Manual (operator after #1040 merges):

  1. git fetch upstream && git rebase upstream/main — should be a clean fast-forward / mechanical merge with no conflicts (this PR only modifies test_hooks.py; fix(langfuse): chown postgres/clickhouse data dirs to image uids on Linux #1040 does not touch that file)
  2. pytest dream-server/extensions/services/dashboard-api/tests/test_hooks.py::TestLangfuseManifestHook -v → both tests PASS
  3. (Adversarial) Rename hooks/post_install.sh to hooks/postinstall.sh and re-run → second test fails with the actionable diagnostic message

Review

Critique Guardian APPROVED (post-warning-fix re-review). The two warnings from the first CG pass — stale "PR #1040" mentions in source and missing .get() defensive style — were both addressed before commit.

Platform Impact

Platform Impact
macOS / Linux / Windows (WSL2) Pure pytest. CI runs on Linux. Test logic platform-neutral (Path manipulations + YAML parse).

Add TestLangfuseManifestHook to dashboard-api/tests/test_hooks.py. Two
narrow assertions guarantee that langfuse's manifest declares
service.setup_hook = "hooks/post_install.sh" AND that the referenced hook
file exists on disk. If either drifts (file renamed, manifest field
removed, hook deleted), _validate_hook_path returns None and
_handle_install silently skips the hook — langfuse silently regresses to
the broken Linux postgres uid mismatch behaviour with no CI signal. The
test catches that.

Uses defensive manifest.get("service", {}).get("setup_hook") so a future
regression that deletes the entire service: block produces a clean assertion
failure instead of a less-actionable KeyError. Assertion messages reference
the langfuse postgres uid 70 install fix conceptually rather than a
transient PR number, so the wording remains accurate after rebase.
@yasinBursali
Copy link
Copy Markdown
Contributor Author

MUST MERGE AFTER #1040. This PR's tests assert that langfuse/manifest.yaml declares service.setup_hook and that langfuse/hooks/post_install.sh exists — both added by #1040. Tests fail on bare upstream/main until #1040 lands. Rebasing post-#1040 is mechanical (no file overlap).

@Lightheartdevs
Copy link
Copy Markdown
Collaborator

Audit follow-up: keep draft / stack on the implementation.

The structural guard is useful, but this branch currently asserts that the Langfuse hook implementation already exists while the branch does not include it. Please retarget/stack this on the PR that adds the hook file and service.setup_hook, or include the implementation here so the tests pass on the branch itself.

@Lightheartdevs
Copy link
Copy Markdown
Collaborator

This is still draft and still red on its own branch. I re-ran the focused hook tests locally:

python -m pytest dream-server/extensions/services/dashboard-api/tests/test_hooks.py -q

Result: 2 failed, 11 passed. The two new assertions fail because this branch does not include the Langfuse service.setup_hook = hooks/post_install.sh manifest change or the extensions/services/langfuse/hooks/post_install.sh file. That means the PR adds a regression guard for a fix that is not present in the branch.

Please stack/rebase this after the Langfuse hook implementation is merge-ready, or include the implementation here, then undraft and rerun the API job. As-is it cannot merge because it intentionally redlines CI.

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