Skip to content

fix(test): use canonical temp path for verification cleanup exemption - #76742

Open
686f6c61 wants to merge 1 commit into
NousResearch:mainfrom
686f6c61:fix/70797-macos-verification-tmp-test
Open

fix(test): use canonical temp path for verification cleanup exemption#76742
686f6c61 wants to merge 1 commit into
NousResearch:mainfrom
686f6c61:fix/70797-macos-verification-tmp-test

Conversation

@686f6c61

@686f6c61 686f6c61 commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

Summary

test_nonrecursive_verification_artifact_cleanup_is_not_dangerous failed on macOS because it fed a literal /tmp/... operand while the exemption compares against realpath(gettempdir()) (/private/tmp on macOS).

Changes

  • Build the exempted operand from os.path.realpath("/tmp") under the mocked gettempdir
  • Keep the neighboring symlink test as the fail-closed contract

No production code change — the implementation is correct; the test was wrong.

Test plan

  • pytest tests/tools/test_approval.py::TestDetectDangerousRm::test_nonrecursive_verification_artifact_cleanup_is_not_dangerous
  • neighboring symlink exemption test

Fixes #70797

The exemption is intentionally realpath-only. On macOS /tmp is a symlink
to /private/tmp, so build the operand from realpath(gettempdir()) rather
than a literal /tmp path.

Fixes NousResearch#70797
@alt-glitch alt-glitch added type/test Test coverage or test infrastructure P3 Low — cosmetic, nice to have tool/terminal Terminal execution and process management area/auth Authentication, OAuth, credential pools needs-decision Awaiting maintainer decision before any implementation labels Aug 2, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related: #70415 proposes production support for the raw top-level macOS temp alias. This PR instead corrects the fixture to the existing canonical-only contract. A maintainer decision is needed; these are competing policies, not duplicates.

@teknium1 teknium1 added sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users labels Aug 2, 2026
@686f6c61

686f6c61 commented Aug 2, 2026

Copy link
Copy Markdown
Contributor Author

Noted on the policy fork with #70415.

This PR intentionally does not change production exemption behavior: the verifier already writes under realpath(gettempdir()), and the neighboring symlink test pins fail-closed for non-canonical operands. The fixture now matches that contract so macOS CI/local runs are consistent.

Happy to close/adjust if maintainers prefer the #70415 production alias path instead.

@webdevtodayjason

Copy link
Copy Markdown
Contributor

This is the fix from #70797, thanks for splitting it out. Same one-line change #55946 already carries inside the kanban work, so the two are the same fix and not competing. Once this lands I'll rebase #55946 onto it, the conflict is just this one line.

@686f6c61

686f6c61 commented Aug 2, 2026

Copy link
Copy Markdown
Contributor Author

Thanks — glad this lines up with #55946.

Happy for this small fixture-only fix to land first so you can rebase the kanban PR on top with a one-line conflict (or none). No further changes planned on this head.

@alt-glitch alt-glitch removed the sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows label Aug 2, 2026
@GottZ

GottZ commented Aug 3, 2026

Copy link
Copy Markdown

This was generated by AI during triage.

Summary

One PR directly addresses #70797. #76742 changes only the failing test fixture, replacing the literal macOS /tmp operand with its canonical path while preserving the existing canonical-only production exemption.

Related pull requests

Duplicates

The same fixture correction is reportedly also present in #55946, so that overlapping line should be removed from #55946 when it is rebased; #70415 is not a duplicate because it proposes a different production policy.

Suggested consolidation

Keep #76742 open with a salvage path: retain its focused fixture-only canonicalization because that diff directly addresses the reported macOS failure while preserving the existing security contract. Do not treat #70415 as a duplicate; remove the duplicate fixture change from #55946 rather than closing its broader kanban work.

Complex graph

flowchart LR
    classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
    classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
    classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
    classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
    classDef best stroke-width:3px,stroke:#b45309
    classDef target stroke-width:3px,stroke:#4338ca
    I70797(["issue #70797 (open)"])
    P76742["PR #76742 (open)"]
    P76742 -->|best fix| I70797
    class I70797 open
    class P76742 open
    class P76742 best
    class P76742 target
    click I70797 "https://github.com/NousResearch/hermes-agent/issues/70797"
    click P76742 "https://github.com/NousResearch/hermes-agent/pull/76742"
Loading

Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label).

Cross-PR triage: Reviewed 1 pull request and 1 issue in this complex. Each diff was read against this issue; Assessment working set: 1 kB of PR diffs, 4 kB of issue/PR text, 1 kB of discussion (4 comments), 2 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related: #70415 changes production behavior to recognize the raw macOS temp alias, while this PR keeps canonical-only behavior and corrects the fixture. This is a policy choice, not a duplicate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/auth Authentication, OAuth, credential pools comp/tools Tool registry, model_tools, toolsets needs-decision Awaiting maintainer decision before any implementation P3 Low — cosmetic, nice to have sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users tool/terminal Terminal execution and process management type/test Test coverage or test infrastructure

Projects

None yet

5 participants