Refactor: extract shared count-gated handler scaffold for safe-output handlers

[Copilot]

25+ safe-output handler modules duplicate the same max-count gating scaffold (processedCount state, limit check, error envelope) and staged-mode resolution. This increases maintenance cost and drift risk when behavior changes are needed.

Changes

• New handler_scaffold.cjs — createCountGatedHandler({ handlerType, setup }) factory that owns:
  • maxCount extraction from config (default 10, using || to match prior handler semantics)
  • isStaged resolution via isStagedMode(config)
  • processedCount closure state
  • Gate check with standardized { success: false, error: "Max count of N reached" } response
• Migrated first two cohorts per issue plan:
  • add_labels.cjs, remove_labels.cjs (label handlers)
  • assign_to_user.cjs, unassign_from_user.cjs (assignee handlers)
• 10 new scaffold tests covering limit enforcement, independent instance counts, delegation, failed-call counting, falsy max edge case, and isStaged propagation

Usage

const { createCountGatedHandler } = require("./handler_scaffold.cjs");

const main = createCountGatedHandler({
  handlerType: "add_labels",
  setup: async (config, maxCount, isStaged) => {
    const githubClient = await createAuthenticatedGitHubClient(config);
    // ... handler-specific init ...
    return async function handleAddLabels(message, resolvedTemporaryIds) {
      // ... handler-specific logic (no max-count or staged-mode boilerplate) ...
    };
  },
});

All 92 existing handler tests + 10 new scaffold tests pass. No behavior changes — identical error messages, response shapes, and count semantics. Remaining handlers can be migrated incrementally in follow-up cohorts.

[Copilot]

Pull request overview

This PR introduces a shared “count-gated” handler scaffold to eliminate duplicated max-count enforcement logic across safe-output handlers, and migrates the first set of handlers to use it.

Changes:

• Added createCountGatedHandler({ handlerType, setup }) to centralize max extraction, processedCount tracking, and standardized limit responses.
• Migrated four handlers (add_labels, remove_labels, assign_to_user, unassign_from_user) to the new scaffold.
• Added handler_scaffold unit tests covering gating behavior and delegation.

Show a summary per file

File	Description
actions/setup/js/handler_scaffold.cjs	New shared factory encapsulating max-count gating + standardized limit error envelope.
actions/setup/js/handler_scaffold.test.cjs	New tests verifying scaffold defaulting, delegation, and limit behavior.
actions/setup/js/add_labels.cjs	Refactored to use shared scaffold instead of local processedCount/limit boilerplate.
actions/setup/js/remove_labels.cjs	Refactored to use shared scaffold instead of local processedCount/limit boilerplate.
actions/setup/js/assign_to_user.cjs	Refactored to use shared scaffold instead of local processedCount/limit boilerplate.
actions/setup/js/unassign_from_user.cjs	Refactored to use shared scaffold instead of local processedCount/limit boilerplate.

Copilot's findings

Tip

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

• Files reviewed: 6/6 changed files
• Comments generated: 2

[Copilot]

maxCount is derived with config.max ?? 10, which changes semantics vs the previous duplicated scaffold in some migrated handlers that used config.max || 10 (e.g., remove_labels/unassign_from_user). With the new logic, max: 0 (or an empty string) will no longer fall back to 10 and will effectively disable processing, which contradicts the PR description of “no behavior changes.” Consider matching the prior behavior (use || 10) or explicitly validating/parsing config.max to a positive integer before using it.

Suggested change

	const maxCount = config.max ?? 10;
	const maxCount = config.max || 10;

[Copilot]

The scaffold tests cover defaulting when config.max is missing, but they don’t cover the edge case where config.max is present but falsy (e.g., 0 or ""). This is exactly where ?? vs || semantics diverge and where regressions are most likely. Add an explicit test to lock down the intended behavior for max: 0 (and/or empty string) once the scaffold’s max parsing/defaulting behavior is finalized.

Suggested change

	it("should preserve maxCount when explicitly set to 0", async () => {
	const setupSpy = vi.fn().mockResolvedValue(async () => ({ success: true }));
	const factory = createCountGatedHandler({
	handlerType: "test_handler",
	setup: setupSpy,
	});
	const config = { max: 0 };
	await factory(config);
	expect(setupSpy).toHaveBeenCalledWith(config, 0);
	});

[github-actions]

🧪 Test Quality Sentinel Report

Test Quality Score: 69/100

⚠️ Acceptable — test quality is good overall, with minor improvements suggested

Metric	Value
New/modified tests analyzed	8
✅ Design tests (behavioral contracts)	6 (75%)
⚠️ Implementation tests (low value)	2 (25%)
Tests with error/edge cases	5 (62.5%)
Duplicate test clusters	0
Test inflation detected	YES (154 test lines / 54 prod lines = 2.85:1)
🚨 Coding-guideline violations	None

---

Test Classification Details

All 8 tests (click to expand)

Test	File	Classification	Issues Detected
should create a handler factory that returns a function	handler_scaffold.test.cjs:16	⚠️ Implementation	Only asserts typeof handler === "function" — verifies structure, not behavior
should pass config and maxCount to setup function	handler_scaffold.test.cjs:26	⚠️ Implementation	Only asserts spy was called with args; no assertion on observable output of the returned handler
should default maxCount to 10 when not specified	handler_scaffold.test.cjs:40	✅ Design	Tests default-value behavioral contract (edge case: empty config)
should delegate to handleItem when under the limit	handler_scaffold.test.cjs:52	✅ Design	Verifies result passthrough from handleItem (observable output)
should respect max count limit	handler_scaffold.test.cjs:67	✅ Design	Core behavioral contract — enforces count gating with correct error envelope
should log warning when max count is reached	handler_scaffold.test.cjs:94	✅ Design	Observable side effect: warning is part of the public contract
should count failed handler calls toward the limit	handler_scaffold.test.cjs:107	✅ Design	Edge case: failed calls count toward limit — behavioral invariant
should maintain independent counts across separate factory invocations	handler_scaffold.test.cjs:130	✅ Design	Closure isolation — state independence is a behavioral contract

---

Flagged Tests — Requires Review

⚠️ should create a handler factory that returns a function (handler_scaffold.test.cjs:16)

Classification: Implementation test
Issue: The sole assertion is expect(typeof handler).toBe("function"). This checks a structural property rather than any observable behavior.
What design invariant does this test enforce? None — it only confirms the return type, which is trivially implied by calling await handler(...) in every other test.
What would break if deleted? No behavioral regression would go undetected; every other test already implicitly requires the factory to return a callable.
Suggested improvement: Replace with an assertion on observable behavior upon invocation, e.g. verify the handler returns a { success: true } result when the limit is not reached.

---

⚠️ should pass config and maxCount to setup function (handler_scaffold.test.cjs:26)

Classification: Implementation test
Issue: Spies on the setup callback and only asserts setupSpy.toHaveBeenCalledWith(config, 7). There is no assertion on what the returned handler actually does with that config.
What design invariant does this test enforce? That config is forwarded to setup — an internal wiring detail, not an observable output.
What would break if deleted? Only if the internal delegation path changes; a bug in how config affects the handler's runtime behavior would not be caught by this test alone.
Suggested improvement: After calling factory(config), invoke the returned handler and assert on its output or side effects. The config-forwarding invariant becomes implicit through correct behavior (e.g., the resolved max: 7 causes gating after 7 calls rather than 10).

---

Language Support

Tests analyzed:

• 🐹 Go (*_test.go): 0 tests — no Go test files changed in this PR
• 🟨 JavaScript (*.test.cjs, *.test.js): 8 tests (vitest) in actions/setup/js/handler_scaffold.test.cjs

---

Verdict

✅ Check passed. 25% of new tests are implementation tests (threshold: 30%). The two flagged tests are low-value but do not push the ratio above the failure threshold. Addressing them would improve the score to ~86/100.

Note on test inflation: The test file adds 154 lines for 54 lines of production code (2.85:1 ratio, exceeding the 2:1 threshold). The production code is intentionally concise (a shared scaffold), so the higher ratio is partially justified by the need to test several behavioral dimensions of the count-gating logic. Still, removing the two low-value tests would improve the ratio to ~2.5:1 and eliminate the inflation penalty.

---

📖 Understanding Test Classifications

Design Tests (High Value) verify what the system does:

• Assert on observable outputs, return values, or state changes
• Cover error paths and boundary conditions
• Would catch a behavioral regression if deleted
• Remain valid even after internal refactoring

Implementation Tests (Low Value) verify how the system does it:

• Assert on internal function calls (mocking internals)
• Only test the happy path with typical inputs
• Break during legitimate refactoring even when behavior is correct
• Give false assurance: they pass even when the system is wrong

Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators.

🧪 Test quality analysis by Test Quality Sentinel · ● 607.7K · ◷

[github-actions]

✅ Test Quality Sentinel: 69/100. Test quality is acceptable — 25% of new tests are implementation tests (threshold: 30%). Two low-value tests were flagged (should create a handler factory that returns a function and should pass config and maxCount to setup function) — see the comment above for suggested improvements. Core behavioral contracts (count gating, error envelopes, closure isolation, warning side effects) are well covered.

[pelikhan]

@copilot review all comments

[Copilot]

@copilot review all comments

Addressed both review comments in e717d42:

• Changed config.max ?? 10 → config.max || 10 in the scaffold to match the prior behavior of remove_labels and unassign_from_user (falsy values like 0 fall back to 10).
• Added a test for the max: 0 edge case to lock down the intended behavior.

[pelikhan]

@copilot common to all safe outputs, should be part of the wrapper

[Copilot]

Moved isStagedMode(config) into the scaffold in 445f696. The scaffold now calls it once and passes isStaged as the third parameter to setup(config, maxCount, isStaged). All four migrated handlers no longer import or call isStagedMode directly.