Skip to content

fix(messages): enforce admission scope on Advisor models - #6617

Merged
lidge-jun merged 4 commits into
lidge-jun:devfrom
luvs01:fix/native-advisor-model-scope
Oct 5, 2026
Merged

lidge-jun merged 4 commits into
lidge-jun:devfrom
luvs01:fix/native-advisor-model-scope

Conversation

@luvs01

@luvs01 luvs01 commented Oct 5, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

A model-scoped data key can select an allowed Messages executor while a preserved Advisor tool names a different, separately billed model. Carry the admission scope into managed native Messages and check active Advisor destinations before credential preparation or dispatch.

  • Apply the existing bare/qualified model-scope rules using the configured provider name for both key and OAuth lanes.
  • Resolve top-level and inline declarations, replacement/removal/re-offer, deferred tools, and persistent clear_at: "never" messages.
  • Leave custom schemas, tool arguments, historic results, provider-only scopes and unrestricted admissions unchanged. Denials use the existing HTTP 403 model-scope response.

This follows the managed native behavior in #6559. The affected new allowance is not in main/latest release 2.77.0. Anthropic's Advisor contract defines the nested model as a billed sub-inference; its inline tool contract defines the lifecycle rules used here. No live account entitlement or provider charging is claimed.

Verification

Base: e53c8077e40b6f540bf1e5d5791a55282466bfac. Head: e2cd8cf72b06cc942d0ff3d498e5367bc9425e9b. Windows, Bun 1.4.2.

  • Production-handler regression before the fix: 11 passed, 10 failed; the denied destinations reached the recording synthetic transport.
  • bun scripts/test.ts --parallel=1 tests/claude-integration/messages-native-scope.test.ts tests/claude-integration/messages-native.test.ts tests/claude-integration/messages-native-oauth.test.ts tests/server/api-key-model-scope.test.ts tests/test-layout-tooling.test.ts --timeout 30000: 120 passed, 0 failed, 1275 assertions.
  • A fresh independent review found the clear_at: "never" bypass and removal regression. Three added cases failed before correction. The matrix above includes the correction.
  • The final scope fixture uses the documented Sonnet 5.5/Opus 5.5 pair supporting both Advisor and inline changes. Re-running tests/claude-integration/messages-native-scope.test.ts after that fixture-only adjustment: 24 passed, 0 failed, 88 assertions. Runtime source is identical to the 120-test run.
  • TypeScript, privacy, structure, file-size and whitespace checks passed. Docs frozen install/build passed: 561 pages, 78,082 internal links, with the generated guidance read back.
  • Default changed mode failed on the shared shallow checkout's merge base. An exact-base import-connected run hit the explicitly configured local 60-second wrapper limit (exit 124). It remains incomplete. Concurrent worktree contention makes the full local selection impractical; the focused exception and wider hosted coverage are explicit.

No paid provider call, installed runtime, credential selection or live policy was changed. Independent local review is not project-maintainer approval. Keep Draft pending the required admission/credential boundary review and final-head validation.

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • Docs or release notes were updated when needed.
  • Required project-maintainer security review of admission and billed destinations is complete.

Review readiness checklist

  • Required validation passed with exact commands and scope recorded.
  • Current dev ancestry rechecked before readiness.
  • All correct review findings resolved.
  • Ready for review.

Maintainer update (lane B5), head 113bc200aafdb7ed319d560a4e754cc147a8f9f8

  • Merged current dev. cdacd4f0ed makes the Advisor scope check fail closed for system messages whose clear_at is not never: additions always count, only permanent removals withdraw, and a temporary replacement keeps the shadowed declaration. 113bc200aa changes only docs and fixtures, which now use the documented next_user_message value.
  • No local tests were run, per owner instruction. Static checks at this head: bun run typecheck, bun run structure:check, bun run privacy:scan, and bun scripts/file-size-ratchet.ts all exit 0. Behavioral evidence comes from hosted PR CI at this head.
  • Independent security review at this head: PASS (see PR comment).

@coderabbitai

coderabbitai Bot commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Note

Currently processing new changes in this PR. This may take a few minutes, please wait...

⚙️ Run configuration
  • Configuration used: Repository: lidge-jun/opencodex/.coderabbit.yaml
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: 581cc83d-5721-41ed-bd8b-38c8b709b2c8
📥 Commits

Reviewing files that changed from the base of the PR and between 37189ca and 113bc20.

📒 Files selected for processing (8)
  • docs-site/src/content/docs/guides/remote-hub.md
  • scripts/test-layout/layout.json
  • src/server/claude-messages.ts
  • src/server/messages-native-scope.ts
  • src/server/messages-native.ts
  • structure/data-planes/protocol-paths.md
  • tests/claude-integration/messages-native-scope.test.ts
  • tests/fixtures/test-layout-expected.json
 ____________________________________________________________________________________________________________________________________________
< Every great developer you know got there by solving problems they were unqualified to solve until they actually did it. - Patrick McKenzie >
 --------------------------------------------------------------------------------------------------------------------------------------------
  \
   \   \
        \ /\
        ( )
      .( o ).
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Oct 5, 2026

Copy link
Copy Markdown
Contributor

✅ Deterministic PR hygiene checks passed.

@github-actions github-actions Bot added the bug Something isn't working label Oct 5, 2026
A system message with a temporary clear_at still covers the current turn.
Count its tool additions, honor only permanent removals, and keep a
declaration shadowed by a temporary replacement in scope.
@lidge-jun

Copy link
Copy Markdown
Owner

Independent security review (lane B5) — bound to head 113bc200aafdb7ed319d560a4e754cc147a8f9f8

Verdict: PASS (independent gpt-6.1-sol reviewer, static review of the full PR diff against dev; no local tests were run, per owner instruction).

Maintainer changes pushed to this branch:

  • Merged current dev (37189ca3c6).
  • cdacd4f0ed: a system message whose clear_at is not never no longer bypasses the Advisor scope check. Additions in it are always checked, only a permanent removal withdraws a declaration, and a temporary replacement keeps the declaration it shadows in scope. Cases were added to the existing messages-native-scope.test.ts.
  • 113bc200aa: docs and fixtures use the documented next_user_message value and describe this as defensive handling, because Anthropic rejects tool changes in turn-scoped system messages.

Review findings at the final head:

  • handleClaudeMessages is the only managed native dispatch entry. It passes the admission scope, and the Advisor check runs before body preparation, OAuth selection, spend reservation, and any upstream send. Credential rotation keeps the checked provider, model, and declarations.
  • The 403 body names only the requested executor selector, the same as existing admission refusals. It does not reveal the Advisor model or the resolved destination.
  • The change adds no logging of request bodies or credentials. No false 403 was found for a documented valid request.

Static checks at the head: bun run typecheck, structure:check, privacy:scan, and bun scripts/file-size-ratchet.ts all exit 0.

@lidge-jun
lidge-jun marked this pull request as ready for review October 5, 2026 15:09
@lidge-jun

Copy link
Copy Markdown
Owner

Maintainer integration (lidge-jun, dev only) — 2026-10-05 bug triage round 2, lane B5.

  • Head: 113bc20
  • CI at this head: PR CI 37318207845; every executed job passed.
  • Review: enforce admission scope on Advisor models through native Messages; lane fix makes clear_at handling fail closed; independent sol security review PASS at this head; readiness ticked by maintainer with hosted-CI-only note.
  • No local tests were run, per owner instruction; hosted CI is the evidence.
  • scripts/ci/assert-mergeable-review.sh --maintainer-integration: OK.

@lidge-jun
lidge-jun merged commit e2b6eef into lidge-jun:dev Oct 5, 2026
36 of 38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants