Skip to content

Add token passthrough feature - #12

Merged
bettercallsaulj merged 15 commits into
mainfrom
feature_token_protect
Jul 31, 2026
Merged

Add token passthrough feature#12
bettercallsaulj merged 15 commits into
mainfrom
feature_token_protect

Conversation

@bettercallsaulj

Copy link
Copy Markdown
Collaborator

No description provided.

@bettercallsaulj
bettercallsaulj force-pushed the feature_token_protect branch from 7045a5f to 2e1e79a Compare July 28, 2026 03:53

@dIvYaNshhh dIvYaNshhh left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ctypes marshalling in here is careful work — _AgentOptionsStorage gets the buffer-lifetime problem right in a way that's easy to get wrong, _bind_optional_agent_options_symbol is the per-symbol try/except that #10's big shared block should have been, and _missing_options_symbol_message() is exactly the actionable error I was asking for on that PR. Good stuff.

Three things I think block merge:

1. The 12.9 MB of committed binaries. create_by_url_gateway (8.76 MB) and create_by_url_server (4.14 MB) are checked in as executables. I pulled the magic bytes — they're Mach-O 64-bit x86_64, so they won't run on Linux CI and need Rosetta on Apple Silicon, which makes create_by_url_run.sh unrunnable for most of the team. There's also no source or build recipe in the repo, so there's no way to review what they actually do — that's a supply-chain question I can't sign off on.

Could these be built from third_party/gopher-orch in the script, or pulled as release assets? Once they're in history they're there permanently.

2. The native-library search reorder (inline below). It ships to every PyPI user, not just developers, and turns "process happens to run from a writable directory" into arbitrary native code execution.

3. Empty access_token produces Authorization: Bearer , and the shipped example hits that path by default.

Plus a question I couldn't answer from the diff: whether the native side deep-copies the options struct at creation or retains the pointer. If it retains it, the storage is freed too early. Details inline.

Smaller stuff: Optional[object] defeats mypy on every new signature, runtime options get normalised at two different layers depending on which factory you use, and ffi/library.py now imports the public config layer. All noted inline.

Base is iml_new_apis, so this can't land until #10 does — and it shouldn't land alone regardless, because of the --remote submodule change (#13 reverts it).

Comment thread gopher_mcp_python/ffi/library.py Outdated
Comment thread gopher_mcp_python/config.py Outdated
Comment thread gopher_mcp_python/ffi/library.py
Comment thread gopher_mcp_python/agent.py
Comment thread gopher_mcp_python/ffi/library.py
Comment thread build.sh Outdated
Comment thread examples/header/create_by_url_run.sh Outdated
Comment thread examples/header/create_by_url.py Outdated
@dIvYaNshhh dIvYaNshhh mentioned this pull request Jul 29, 2026
@bettercallsaulj
bettercallsaulj force-pushed the feature_token_protect branch 2 times, most recently from bb91598 to a46cfc5 Compare July 31, 2026 06:27

@dIvYaNshhh dIvYaNshhh left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed at a46cfc58. Eight of nine addressed, and a few went past what I asked. Resolving those threads.

Confirmed fixed:

  • Search order (2c21fa03) — installed platform package back at position 1, cwd dev paths after it, and the comment now points at GOPHER_MCP_PYTHON_LIBRARY_PATH. Hijack vector closed.

  • Empty access_token (fc5ad5b0) — fixed in both places: access_token == "" normalises to None, and _normalize_headers gates on if access_token:. Covered by a test.

  • Options struct lifetime (229ad3ff) — you went and got the answer to the question I couldn't resolve from the diff, and put it where the next reader will find it:

    Native BuildAgentOptions deep-copies this struct into C++ strings/maps during creation, so this storage only needs call-duration lifetime.

    That, plus the comment explaining why _bytes is pinned, is exactly right. Thanks for chasing the C side rather than just asserting it.

  • Normalisation at two layers (c012d442) — done once at the agent boundary now, so both factory paths fail at the same point.

  • Layering (28764f27) — the new gopher_mcp_python/runtime_options.py actually breaks the ffi → config dependency instead of papering over it.

  • TypingRuntimeOptionsInput replaces Optional[object]; zero occurrences left in agent.py, so mypy can see through it.

  • Submodule pinning (6f88270e) — GOPHER_ORCH_TRACK_REMOTE=1 opt-in, pinned by default.

  • Temp dir (c75d5e8c).

Suite at the tip of the stack: 151 passed, 116 skipped, 3 failed — all three failures reproduce identically on origin/main (they need ./build.sh), so nothing regressed.

One item left, and it's the one I'd hold merge on: the 12.9 MB of committed binaries — no commit touches them. Detail inline.

Separately: the description is still empty. With a new public module, a new FFI struct, and a build-script behaviour change in here, a few lines of scope would help the next reviewer.

Comment thread examples/header/create_by_url_run.sh Outdated

@dIvYaNshhh dIvYaNshhh left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

de69016c closes the last item — resolving the thread. Nothing outstanding from me on this PR now.

You took option 1 and did it properly:

  • Both binaries deleted, and create_by_url_run.sh now builds header_access_token_create_by_url_{server,gateway} from third_party/gopher-orch via cmake --build.
  • build_header_binaries() only runs when the binaries are absent, so repeat runs don't pay for a rebuild.
  • It fails with an actionable message when the submodule isn't initialized, and auto-inits the nested gopher-mcp when that's what's missing.
  • Core count falls back sysctl -n hw.ncpu -> nproc -> 4, so it works on macOS and Linux.

Net effect: the script now runs on Linux and Apple Silicon, where before it couldn't run at all.

Suite at the tip of the stack: 153 passed, 116 skipped, 3 failed — the three failures reproduce identically on origin/main (they need ./build.sh), so nothing regressed.


One thing for whoever clicks merge — not a code change. Deleting the files fixes the tree at HEAD, but the blobs are still reachable in this branch's history:

$ git rev-list --objects feature_token_protect --not origin/main | ...
  4.0 MB  examples/header/create_by_url_server
  8.4 MB  examples/header/create_by_url_gateway

added in e0c4bc47, removed in de69016c. The repo currently has allow_merge_commit, allow_squash_merge and allow_rebase_merge all enabled — and with a merge commit or rebase-merge those 12.4 MB land in main's history permanently, even though no commit on main would have them in its tree.

Squash-merging this PR avoids it entirely (the squash commit carries only the final tree). Worth agreeing before merge — cheap now, needs a history rewrite later. Same applies to #13 and #14, which sit on this branch.

RahulHere added 15 commits July 31, 2026 17:23
Add ctypes bindings for gopher_orch_agent_options_t and optional *_with_options agent creation symbols.

Route non-empty runtime options through the new native symbols and fail clearly when the loaded native library is too old.

Cover options struct conversion, fallback behavior without runtime options, and missing-symbol errors with focused tests.
Forward configured runtime options into API-key and JSON native agent creation.

Add runtime_options parameters to the direct server, gateway, and URL factory helpers.

Cover public passthrough and old-native-library errors with fake-library tests.
Add a Python header example showing create_with_url with runtime access_token and headers options.

Default the MCP URL to the local gateway address used by the token-flow examples and keep examples/api unchanged.
Add create_by_url_run.sh for the Python header example using the copied local gateway and MCP server helpers.

The runner releases ports 5000/5001, starts the helpers with GOPHER_SDK_TEST token-flow logging, runs deterministic MCP curl calls, and optionally runs the Python SDK client when ANTHROPIC_API_KEY is available.

Copy create_by_url_gateway and create_by_url_server helper executables from the gopher-mcp-js header example.
Summary:
- search installed platform native packages before cwd/local development paths in both FFI loaders
- keep local build loading behind explicit environment overrides such as GOPHER_MCP_PYTHON_LIBRARY_PATH
- update search-order tests and runtime-options fake signature for the current API

Verification:
- git diff --check
- python3 -m pytest tests/test_library_search_paths.py tests/ffi/auth/test_loader.py tests/test_ffi.py tests/test_ffi_runtime_options.py -q
Summary:
- normalize empty access_token values to absent across mapping, object, and builder paths
- avoid generating Authorization: Bearer with an empty token
- omit empty access_token from the dynamic header create_by_url example
- add config and FFI regression coverage for empty access tokens

Verification:
- git diff --check
- python3 -m pytest tests/test_config.py tests/test_ffi_runtime_options.py tests/test_agent_runtime_options.py -q
Summary:
- explain why encoded header bytes are pinned while populating c_char_p fields
- document that native BuildAgentOptions deep-copies runtime options during agent creation
- clarify that Python storage only needs to survive the FFI create call

Verification:
- git diff --check
- python3 -m pytest tests/test_ffi_runtime_options.py -q
Summary:
- add an explicit RuntimeOptionsInput type for public runtime option signatures
- normalize direct factory runtime options before FFI calls to match builder behavior
- reject invalid direct-factory runtime options before native invocation
- update tests to assert normalized options instead of raw object passthrough

Verification:
- git diff --check
- python3 -m pytest tests/test_agent_runtime_options.py tests/test_config.py tests/test_ffi_runtime_options.py tests/test_ffi.py -q
Summary:\n- move runtime option types and normalization into a shared low-level module\n- keep config as a consumer while preserving public exports\n- raise AgentError for direct FFI missing-symbol failures\n- update FFI tests for SDK error hierarchy
Summary:\n- restore parent-recorded submodule SHAs as the default build behavior\n- add GOPHER_ORCH_TRACK_REMOTE=1 for explicit branch-tip tracking\n- apply the same pinned/remote choice to nested gopher-mcp updates
Summary:\n- create private mktemp log directories and remove logs after successful runs\n- stop killing arbitrary listeners on default ports\n- auto-select local ports and retry on bind failures\n- verify Authorization reachability without depending on exact native token log formatting
Summary:\n- add regression coverage for create_by_url runtime options from env\n- assert empty GOPHER_ACCESS_TOKEN omits access_token and Authorization\n- assert non-empty tokens still use the access_token convenience path
Summary:\n- delete the committed Mach-O header example server and gateway binaries\n- build the required gopher-orch header fixture targets on demand from the runner\n- ignore the old local binary paths so generated artifacts are not re-added\n\nVerification:\n- bash -n examples/header/create_by_url_run.sh\n- python3 -m pytest tests/test_header_create_by_url_example.py
@bettercallsaulj
bettercallsaulj force-pushed the feature_token_protect branch from de69016 to d43201e Compare July 31, 2026 09:24
@bettercallsaulj
bettercallsaulj changed the base branch from iml_new_apis to main July 31, 2026 09:24
@bettercallsaulj
bettercallsaulj merged commit c8483c0 into main Jul 31, 2026
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.

2 participants