fix(coder): address #827 + #828 auto-review findings (security + runtime)

[kovtcharov]

Summary

Six fixes flagged by the auto-review bot: one Critical (security), five Important (two on #827, two on #828, one on both). All 395 tests pass on coder with the fixes.

Changes

Critical (security):

• oss_reuse.py import_with_attribution — path traversal on LLM-controlled dest_path. Now resolves + relative_to(root)-checks; raises AttributionError on escape.

Important:

• oss_reuse.py _validate_license_filter — unknown SPDX ids silently dropped; now raises per CLAUDE.md fail-loudly.
• tools/github.py gh_pr_merge — hardcoded --admin; now gated behind admin_override=False default.
• repo_binding.py webhook round-trip — only did positive check; added wrong-signature + wrong-payload discrimination.
• tools/debug.py add_instrumented_trace — emitted logger.debug(...) requiring pre-bound logger; now inlines __import__('logging') lookup.
• tools/debug.py diff_behavior — git switch - after two detached switches returns to wrong ref; now captures + explicitly restores original HEAD.

Test plan

• pytest tests/coder/ tests/eval/ — 395 pass
• 7 new regression tests in test_fixes_827_828.py cover each fix
• test_add_instrumented_trace_* now asserts the mutated module actually imports (previously asserted only the string was written)

[github-actions]

Summary

Clean, well-scoped follow-up that addresses every auto-review finding from #827 and #828 with matching regression tests. The path-traversal fix in import_with_attribution is the standout — correctly uses resolve() + relative_to() which also blocks symlink escapes, not just ... Each fix is paired with a test that would have caught the original bug, and the add_instrumented_trace test now verifies the mutated module actually imports (the previous assertion only checked string contents). One tiny nit below, otherwise nothing blocking — and since the PR is already merged, these are FYI for future passes.

Issues Found

🟢 Minor — Unused Path import (tests/coder/test_fixes_827_828.py:16)

Path is imported but never referenced — the path-traversal tests use the tmp_path pytest fixture directly (already a Path). isort/flake8 will flag this.

from __future__ import annotations

import pytest

from gaia.coder import oss_reuse

🟢 Minor — Behaviour change to _validate_license_filter return semantics (src/gaia/coder/oss_reuse.py:442)

Before the fix, the function returned requested & PERMISSIVE_LICENSES (intersection — silently widened to allowlist on empty). After, it returns requested directly. For the happy path these are identical once unknown ids raise, but any caller that previously relied on "pass anything, I'll get back a sanitised permissive subset" now gets ValueError. The new behaviour is correct per CLAUDE.md's fail-loudly rule; flagging only so future callers know the contract shifted from "sanitise" to "validate". Test coverage (test_license_filter_still_accepts_known_permissive_subset) confirms the happy path. No change requested.

🟢 Minor — diff_behavior original-HEAD capture happens before git stash (src/gaia/coder/tools/debug.py:471-477)

Ordering is fine today (stash doesn't change HEAD), but if someone later changes git stash to git stash --keep-index or introduces an auto-commit path, HEAD could move before capture. A short comment explaining the ordering invariant would protect the next editor. Not blocking.

Strengths

• Fail-loudly discipline applied consistently — every fix replaces silent degradation (silent license drop, hardcoded --admin, swallowed path escapes, positive-only HMAC check) with a raise or an explicit-opt-in flag, matching the CLAUDE.md rule verbatim.
• Webhook discrimination tests are thoughtful (repo_binding.py:347-364) — both wrong-sig and wrong-payload negative tests catch the "verifier always returns True" failure mode that the positive-only round-trip missed.
• Probe self-containment — __import__('logging').getLogger(__name__) sidesteps the whole "does the target module bind logger?" question and makes the probe safe to inject into any Python file. Test upgrade to actually exec_module the mutated file (test_debug_tools.py:173-181) is exactly the right assertion level.
• HEAD restoration via symbolic-ref short-circuit correctly distinguishes branch vs. detached and restores with the matching git switch variant — handles the two-detached-switches case that broke git switch -.

Verdict

Approve with suggestions — the unused-import cleanup is the only mechanical change worth making; everything else is either informational or defensive. Nice surgical PR.