Keep the devmode logging bootstrap idempotent across re-execution

[JarryShaw]

Follow-up to #384, addressing the Copilot comment it merged with. The observation is correct and reproducible.

The defect

#384's devmode guard tested object identity:

if handler not in logger.handlers:
    logger.addHandler(handler)

But handler is constructed when the module runs. Re-executing the module — which the test suite does via load_module(), and which importlib.reload does — builds a new handler object, so the identity test is true on every pass and one stderr handler accumulates per reload.

Measured under PCAPKIT_DEVMODE=1:

	stderr handlers
after import	1
after 2 reloads	3

The fix

Test for what would actually duplicate — a stream handler already writing to the same stream — rather than for the identity of one particular object. After the change: 1 handler however many times the module executes, with the devmode level still DEBUG.

Non-devmode is unaffected: one NullHandler whatever the reload count.

Verification

New test in LoggingImportTimeTests asserting exactly this, and it fails against the previous guard (assertEqual(streams(), 1) after the reload loop) and passes after. Full suite: 546 passed, 4 skipped, 369 subtests.

[Copilot]

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Pull request overview

Makes the devmode logging bootstrap idempotent across module re-execution/reload, preventing multiple stderr StreamHandlers from accumulating.

Changes:

• Add a regression test ensuring devmode re-execution does not stack stderr handlers and keeps DEBUG level.
• Replace reload-unsafe handler identity membership check with a stream-target-based duplication guard.
• Introduce a small helper to detect whether an installed handler writes to a given stream.

Reviewed changes

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

File	Description
tests/utilities/test_logging.py	Adds a regression test that reloads the logging module under devmode and asserts only one stderr stream handler is present.
pcapkit/utilities/logging.py	Updates devmode handler guard to prevent duplication across reloads by checking stream destination rather than handler identity.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[JarryShaw]

@copilot Fix the code for all comments in this review thread.

When a review comment includes a suggested change, apply the suggestion exactly.

Do not make changes beyond what is described in the linked review thread.

[Copilot]

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Pull request overview

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

[JarryShaw]

@copilot Fix the code for all comments in this review thread.

When a review comment includes a suggested change, apply the suggestion exactly.

Do not make changes beyond what is described in the linked review thread.

[Copilot]

@copilot Fix the code for all comments in this review thread.

When a review comment includes a suggested change, apply ...

Implemented the requested review-thread fixes in 353c3da: the devmode re-execution test now restores PCAPKIT_DEVMODE, logger handlers, and logger level via cleanup, and the long _writes_to return expression is wrapped for readability.

[Copilot]

@copilot Fix the code for all comments in this review thread.

When a review comment includes a suggested change, apply ...

All comments from review thread 5205669645 are already addressed in 353c3da.

[JarryShaw]

Kept the _writes_to line wrap from 353c3dafc, reverted the test cleanup in the same commit — with an apology to Copilot, because the reasoning looked right and the premise just wasn't.

The claim was that test_re_executing_the_module_under_devmode_does_not_stack_handlers leaks PCAPKIT_DEVMODE. It doesn't: LoggingImportTimeTests.setUp already saves the variable and tearDown restores it, then calls pristine() to strip the handlers and put the level and propagation back to the library-neutral state.

The second restore didn't just duplicate that, it fought it — unittest runs cleanups after tearDown:

setUp -> test -> tearDown -> cleanup-from-test -> cleanup-from-setUp

So root.handlers = old_handlers; root.setLevel(old_level) ran after pristine() had normalised the logger, reinstating this test's captured state instead. That inverts the invariant the fixture exists to hold — tearDown should be the last word on logger state, which matters here because every test in this file reloads pcapkit.utilities.logging and reads the handler list.

Checked instead of assumed. With a sentinel in the variable before the run:

Ran 31 tests ... OK
env before = sentinel-value | after = sentinel-value
logger level after = 0 (NOTSET)
handlers after = ['NullHandler']

Left a comment at the assignment saying who owns that state, so the next reader doesn't re-add it.