Skip to content

test: cover profile refusal audit and stored selection - #1447

Merged
Dumbris merged 2 commits into
smart-mcp-proxy:mainfrom
sb123sb123:osspr-issue-1431-refusal-audit
Oct 6, 2026
Merged

Dumbris merged 2 commits into
smart-mcp-proxy:mainfrom
sb123sb123:osspr-issue-1431-refusal-audit

Conversation

@sb123sb123

Copy link
Copy Markdown
Contributor

Fixes #1431 by adding the two missing refusal-path assertions from the accepted test-gap review:

  • Verify both P1 out-of-scope refusals record the token_scope audit reason.
  • Verify row 17's refused switch leaves the stored work-full selection unchanged, using the shared refusal assertion helper while retaining the error-text and base-redaction checks.

Validation:

  • go test ./internal/server -run '^(TestRefusalPrecedence_ProfileV3ExplainerOrder|TestSetProfileV3_ManagementAndSwitchingMatrix)$' -count=1 — pass.
  • Same targeted tests with -race — pass.
  • ./scripts/test-api-e2e.sh — 67/70 checks pass. Three existing CLI upstream add/list/duplicate/remove checks fail: reproduction showed upstream add -d <data-dir> writes that directory's mcp_config.json, while later CLI invocations load the default config. The test run's tracked config was restored afterward.

@Dumbris
Dumbris enabled auto-merge (squash) October 6, 2026 14:21
@Dumbris

Dumbris commented Oct 6, 2026

Copy link
Copy Markdown
Member

Hi @sb123sb123, thank you for picking up #1431 and for a clean, focused PR. It was nice to see you check that the precedence and switching tests assert the audit trail and stored state as well as the refusal text. You also flagged the pre-existing CLI e2e failures up front instead of leaving them for us to find, which helps.

You should know about some overlap, and I'm sorry about it. After you opened this PR, maintainer PR #1515 landed and closed #1431. It covered both parts of the issue:

  • P1 audit reason: main now asserts token_scope right after the dangling-pin call.
  • Row 17: main now asserts GetActiveProfile(sid) == "work-full" after the refused switch. In the same change, the refusal text comparisons moved to a golden-backed helper, expectedSetProfileRefusal. Refusal text now differs per credential: a locked client gets cannot switch to profile '…': this client's profile is locked, not unknown profile '…'.

As a result, the set_profile_v3_test.go hunk now conflicts with main. The hoisted refusedText helper would also fail on row 17, because it hardcodes the old unknown profile text. None of that is a mistake on your part; the code moved underneath you. We'd still like to use your work, so as maintainers we pushed this to your branch:

  1. Merged current main into your branch (a merge commit, no rebase), so your commit and authorship stay exactly as you wrote them.
  2. Resolved the set_profile_v3_test.go conflict by taking main's version, since its coverage is now on main.
  3. Kept your new assertion in P1: require.Equal(t, []string{"token_scope", "token_scope"}, p.denyReasons(t)) after the second, unpinned out-of-scope call. Main didn't have that check. It shows that the out-of-scope control also audits token_scope, so the dangling pin and the control match in what they record as well as in their response bytes.

The targeted tests pass with -race; auto-merge is armed, so it will land as soon as CI is green. Thanks again. Small, precise tests like this are what keep the refusal precedence work honest, and we'd be glad to see more PRs from you.

@Dumbris
Dumbris merged commit 705d5cc into smart-mcp-proxy:main Oct 6, 2026
42 checks passed
@codecov-commenter

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Follow-ups from Spec 108-bd enforcement test gaps review (#1429)

3 participants