test(hooks): add duplicate hookScope clobber warning test (#544)

[zeroedin]

Summary

• Add failing test asserting EvalFile must surface a warning via EvalWarnings() when a plugin registers the same hook name twice with different scopes
• Add duplicate-hook.js test fixture that registers onContentTransformed twice (pages:false then pages:true)
• Add evalWarnings []string field and EvalWarnings() stub on NodeRuntime
• Update IMPLEMENTATION.md: duplicate detection must happen in bridge.js (JS objects deduplicate keys before Go sees them), bridge returns warnings array in eval response, Go parses and stores via EvalWarnings()

Design decision: last-wins semantics preserved. Warning is informational, not an error. Detection happens at registration time in bridge.js because that is where sequential overwrites of hooks[name] are observable.

1 test fails red (EvalWarnings returns nil — bridge doesn't detect duplicates yet). 1 test passes green (last-wins is current behavior). 0 regressions (161 existing plugin specs pass).

Ref #544

Test plan

• Warning test fails red — go test ./internal/plugin/... --ginkgo.focus="Duplicate"
• Last-wins test passes green
• All existing plugin tests pass — go test ./internal/plugin/... --ginkgo.skip="Duplicate"
• go vet ./internal/plugin/... — clean

🤖 Generated with Claude Code

[Copilot]

Pull request overview

This PR introduces a red (failing) test to drive a new requirement in the Node plugin runtime: emit a warning when a hook scope entry is overwritten due to duplicate hook registration, while keeping last-wins semantics. It also adds a small NodeRuntime test helper and updates the implementation spec to document the expected warning behavior.

Changes:

• Add a new failing spec asserting a “duplicate registration overwrites previous scope” warning (issue #544).
• Add NewNodeRuntimeWithHooks helper for constructing a NodeRuntime with pre-seeded hooks/scopes in tests.
• Update plans/IMPLEMENTATION.md with warn-on-overwrite behavior for Node hook scope parsing.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File	Description
plans/IMPLEMENTATION.md	Documents a warn-on-overwrite requirement for Node hook scope clobber handling.
internal/plugin/scope_test.go	Adds a new (currently failing) Ginkgo spec for duplicate hook scope clobber warning behavior.
internal/plugin/export_test.go	Adds a test helper to construct NodeRuntime instances with preset hooks/scopes.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[zeroedin]

Valid. The test constructs a NodeRuntime via helper and calls RegisteredHookDetails() — a pure getter with no side effects. No code path in this test writes to logBuf, so the assertion can never pass regardless of implementation. The test needs to either exercise EvalFile directly or target the actual code path where the warning would fire.

Addressing in rework — the spec and test both need structural changes (see Reviewer P0).

[zeroedin]

Acknowledged. The log.SetOutput approach mutates global state and restores to os.Stderr rather than saving/restoring log.Writer(). Will address in the test rework — the log capture pattern itself may change depending on whether the warning moves to bridge.js or stays in Go.

[zeroedin]

This is the root cause of the P0. The bridge protocol returns hookScopes as a JS object keyed by hook name — Object.keys() deduplicates inherently. Go's EvalFile receives pre-deduplicated data and has no way to detect within-plugin duplicates.

Three options:

1. Move detection to bridge.js — warn at register() time when a hook name is already registered with a different scope
2. Change bridge protocol — send hookScopes as [{name, scope}] array instead of an object, letting Go detect duplicates
3. Refocus on cross-plugin — detect when multiple EvalFile calls register the same hook name with different scopes

Option 1 is simplest and catches the problem at its source. Will rework the spec accordingly.

[zeroedin]

Code Review — PR #554

Run: 20260507-150500-759358c1 | Reviewers: correctness, testing, maintainability, project-standards, previous-comments (5 agents)

Scope: 1 commit (ee979e7d) — 57 insertions, 2 deletions across 3 files (2 test, 1 doc)

Intent: Spec PR — add a failing test to drive implementation of issue #544 (warn on duplicate hookScope clobber). No production code changes.

---

P0 — Critical

#	File	Issue	Reviewers	Confidence
1	scope_test.go:455	Test bypasses EvalFile — cannot drive the correct implementation. NewNodeRuntimeWithHooks constructs a NodeRuntime by directly setting struct fields; RegisteredHookDetails() is a pure getter with no side effects. No code path executed by this test writes to logBuf. The IMPLEMENTATION.md spec says the warning goes in EvalFile (before hookScopes[name] = scope), but EvalFile is never called. No implementation of the spec as written can make this test pass. The implementer's only options: (a) put the warning in RegisteredHookDetails (side effects in a getter — bad design), (b) modify the test helper (violates test immutability), or (c) build additional test infrastructure not anticipated by this PR.	correctness, maintainability, previous-comments	100

P1 — High

#	File	Issue	Reviewers	Confidence
2	scope_test.go:449	scopeA declared but unused — test never sets up actual clobber. scopeA (Data: ["elements"], Glob: /blog/**) is created at line 449 but only referenced via _ = scopeA at line 467. The hookScopes map passed to the helper contains only scopeB. No code assigns scopeA then overwrites with scopeB — the "clobber" the test claims to drive never happens. HaveLen(2) passes trivially because RegisteredHookDetails iterates the hooks slice, not the scopes map.	correctness, maintainability	100
3	IMPLEMENTATION.md:454	Spec text contradicts bridge protocol. The spec says "The warning fires per-plugin, not cross-plugin" but the Go-side check (if hookScopes[name] already exists) can only detect cross-plugin duplicates. Within a single plugin, JS objects can't have duplicate keys — hookScopes arrives pre-deduplicated from bridge.js. The hooks array can carry per-plugin duplicates, but the spec's check targets the map, not the array.	previous-comments	75

---

Testing gaps (not blocking, routed from testing/previous-comments)

• log.SetOutput(&logBuf) mutates global logger state. If Ginkgo runs specs in parallel, log output could leak between tests. Test restores os.Stderr instead of saving/restoring log.Writer(). (Copilot flagged this; unaddressable due to test immutability.)
• No integration test exercises EvalFile with a plugin that registers the same hook name twice.

Dropped findings (5)

• Commit message (#544) in title — matches repo's established pattern (see 8a0f47f)
• parseScopeMap nil guard — pre-existing, not in this PR's diff
• "EvalFile still missing duplicate warning" — expected for a spec PR
• log.SetOutput concurrency — confidence 40, test immutable
• Test helper exposes 3 fields when test uses 2 — YAGNI applies to test helper shrinkage too

---

Verdict: Not ready. The test has a structural mismatch with the spec: it bypasses EvalFile entirely and exercises a pure getter, so no implementation of the spec as written (add a check in EvalFile before hookScopes[name] = scope) can make this test pass. Additionally, scopeA — which represents the scope that should be clobbered — is never assigned to hookScopes, so the duplicate-assignment scenario is never set up.

The spec text also contradicts itself: it asks for a "per-plugin" warning at a check location that only fires cross-plugin, because JS objects deduplicate keys before the bridge sends data to Go.

[zeroedin]

Response to Code Review

All findings accepted. The test and spec have a fundamental structural flaw.

P0 — Test bypasses EvalFile

Correct. NewNodeRuntimeWithHooks + RegisteredHookDetails() exercises a pure getter. No code path writes to logBuf. No implementation of the spec can make this test pass without putting side effects in a getter or modifying the test helper. This is a spec authoring error.

P1 — scopeA unused

Correct. scopeA is declared but only referenced via _ = scopeA. The hookScopes map contains only scopeB. The clobber scenario is never set up.

P1 — Spec contradicts bridge protocol

This is the root cause. Issue #544 says "If a plugin registers the same hook twice with different scopes, the last registration wins silently" — but bridge.js returns hookScopes as a JS object keyed by hook name. JS objects inherently deduplicate keys. Go's EvalFile receives pre-deduplicated data and cannot detect within-plugin duplicates.

Path forward

The spec needs to be rewritten. Three options:

1. Move detection to bridge.js — warn at register() time in the Node bridge when a hook name is already registered with a different scope. Simplest fix, catches the problem at its source.
2. Change bridge protocol — send hookScopes as [{name, scope}] array instead of object, letting Go detect duplicates in EvalFile.
3. Refocus on cross-plugin detection — detect when multiple EvalFile calls (separate plugins) register the same hook name with different scopes. This is a different problem than what fix(hooks): hookScopes map clobbers on duplicate registration in Node runtime #544 describes.

Option 1 is the most direct solution. Will rework the spec, test, and IMPLEMENTATION.md to target the bridge-side warning. Pushing updated commit shortly.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

[zeroedin]

Correct — this is intentional. The stub exists so the test compiles and fails red at the assertion level. Parsing resultMap["warnings"] in EvalFile is implementation work, not spec work. The IMPLEMENTATION.md spec is explicit: "EvalFile parses the warnings array from the eval response and appends each string to r.evalWarnings." The implementer adds the parsing; the test goes green.

[zeroedin]

The PR description is from the original commit (pre-rework). The rework commit a7a2efa removed NewNodeRuntimeWithHooks — the test now uses plugin.NewNodeRuntime() + EvalFile() directly. The description reflects the original approach; the current code is correct.

[zeroedin]

Code Review — PR #554 (Review #2)

Verdict: Ready to merge ✅

Run: 20260508-163000-c78285ee | Reviewers: 6 (previous-comments, correctness, testing, maintainability, project-standards, learnings-researcher) | Head: a7a2efa0

---

Prior findings — all addressed

#	Severity	Finding	Status
C1	P0	Test bypasses EvalFile — NewNodeRuntimeWithHooks + RegisteredHookDetails is a pure getter path, log capture unreachable	Fixed. Test now calls rt.EvalFile() with duplicate-hook.js fixture. Helper removed from export_test.go.
C2	P1	scopeA declared but unused — clobber scenario never constructed	Fixed. Removed entirely. Fixture handles both scope registrations.
C3	P1	Spec targets Go-side detection, but JS objects deduplicate keys before serialization — Go cannot detect within-plugin duplicates	Fixed. IMPLEMENTATION.md now specifies bridge.js detection. Spec text: "Detection must happen in bridge.js because JS objects deduplicate keys before serialization."

New findings — none blocking

No actionable findings. 9 findings from correctness and testing reviewers were stale (reviewed first commit ee979e7d, not final state after rework a7a2efa0). All dropped during synthesis.

Advisory notes

Hooks list double-append (spec gap, not a PR defect): EvalFile appends hook names to r.hooks unconditionally (node.go:169-174). A plugin registering onContentTransformed twice produces two entries in RegisteredHooks(). Both get registered on the HookRegistry, causing the hook to fire twice per dispatch. The spec says "last-wins" for scopes but does not address hooks[] dedup. This is outside the scope of issue #544 (which is specifically about scope clobber warning). If double-fire is unintended, file a separate issue.

EvalFile warnings parsing deferred to implementation: The evalWarnings field and EvalWarnings() accessor are minimal stubs for test compilation. EvalFile does not yet parse a warnings field from the bridge response — that's implementation work alongside the bridge.js changes. The IMPLEMENTATION.md spec is unambiguous about what's needed: "EvalFile parses the warnings array from the eval response and appends each string to r.evalWarnings."

What the rework got right

• Test exercises the real EvalFile → bridge.js → response parsing path
• Two focused test cases: warning detection + last-wins semantics
• Fixture correctly calls alloy.hook() twice with different scope objects (pages: false then pages: true)
• DeferCleanup(rt.Close) for proper teardown
• Warning test will fail red (bridge.js has no detection, EvalFile has no parsing)
• Last-wins test validates existing behavior (PagesScopeAll from second registration)
• IMPLEMENTATION.md accurately describes the bridge-side detection strategy

[zeroedin]

Response to Review #2

Thanks for the clean bill of health.

Re: hooks list double-append (advisory) — Agreed this is a separate concern. The hooks[] slice accumulates raw entries from bridge.js's Object.keys(hooks), which is already deduplicated per-plugin. But across multiple EvalFile calls (multiple plugins registering the same hook), r.hooks would contain duplicates. This is a pre-existing behavior outside #544's scope. Will file a separate issue if it warrants tracking.

Re: EvalFile warnings parsing — Confirmed, the stubs are intentionally minimal. Implementation adds bridge.js detection + Go-side parsing in a single commit.

PR description updated to reflect the rework.