-
Notifications
You must be signed in to change notification settings - Fork 0
feat: Update to SDK 0.6.0 with built-in async polling #78
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughUpgrades Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 14
🤖 Fix all issues with AI agents
In @.monitor_pid:
- Line 1: Remove the runtime PID file ".monitor_pid" from the repository and add
it to .gitignore so it won’t be committed again: delete the tracked file
".monitor_pid" (use git rm --cached or git rm to remove from the index), commit
that deletion, and append an entry for ".monitor_pid" to your .gitignore (or a
suitable pattern like *.pid) to prevent future commits of this local process
artifact.
In @.vm_ip:
- Line 1: The repository currently contains a real VM public IP in the file
named ".vm_ip"; replace that value with a non-production placeholder (e.g.,
"X.X.X.X") and remove the actual IP from the repo, then move the real IP to a
local-only location (for example a gitignored ".vm_ip.local" or environment
variable like VM_IP in a .env) and update docs/README to instruct developers
where to set it; also add the chosen local filename (or .env) to .gitignore so
production IPs are never committed.
In `@azure-benchmark-setup.sh`:
- Around line 41-44: Replace the insecure scp invocation that uses "-o
StrictHostKeyChecking=no" with a workflow that pre-populates the SSH known_hosts
for the target host and then performs scp normally; specifically, run
ssh-keyscan (or fetch the host key out-of-band) for $VM_IP and append it to
~/.ssh/known_hosts (or a temporary known_hosts file), then call scp (the same
command that copies mcpbr-config.yaml to "$ADMIN_USER@$VM_IP:~/") without
disabling StrictHostKeyChecking (or with "-o UserKnownHostsFile=... -o
StrictHostKeyChecking=yes" if using a temp file). Apply the same change to the
other ssh/scp occurrences that currently use StrictHostKeyChecking=no.
- Around line 78-80: The script currently pipes NodeSource's setup script into
bash (curl ... | sudo -E bash -) which bypasses signature verification; replace
that sequence by fetching the NodeSource GPG key (nodesource-repo.gpg.key),
dearmor it into /etc/apt/keyrings/nodesource.gpg, create a signed APT source
entry for node_20.x (using signed-by=/etc/apt/keyrings/nodesource.gpg), run
apt-get update, then install nodejs; remove the curl | bash usage and ensure
ca-certificates, curl, and gnupg are installed beforehand so apt can verify the
repository signature.
In `@BENCHMARK_INFO.md`:
- Around line 8-45: Replace the hardcoded public IP (104.211.53.217) in
BENCHMARK_INFO.md with a placeholder (e.g., ${VM_IP} or <VM_IP>) and update the
example commands (reconnect.sh, monitor-benchmark.sh, ssh azureuser@..., tail -f
~/benchmark.log, and "mcpbr state") to reference a local config or environment
variable (e.g., read from a gitignored .vm_ip file or $VM_IP env var); also add
a brief note pointing users to the .vm_ip file or env var for the real IP and
ensure .vm_ip is listed in .gitignore so the live IP is not committed.
In `@continuous_monitor.sh`:
- Around line 27-28: The SSH invocation that sets STATE_OUTPUT uses -o
StrictHostKeyChecking=no which disables host key verification; instead pre-seed
the target host key into known_hosts (e.g., use ssh-keyscan "$VM_IP" >>
~/.ssh/known_hosts or a temporary known_hosts file) and remove the
StrictHostKeyChecking=no option so the ssh in the STATE_OUTPUT assignment (and
the other similar invocation at lines referencing the same pattern) performs
normal host key verification; ensure you reference ADMIN_USER and VM_IP when
populating the known_hosts entry and point ssh to the proper known_hosts via -o
UserKnownHostsFile if using a temp file.
- Around line 95-141: After parsing STATE_OUTPUT, validate that numeric fields
(COMPLETED, TOTAL, IN_PROGRESS, REMAINING, PERCENT, MCP_RESOLVED,
BASELINE_RESOLVED, MCP_TIMEOUT, BASELINE_TIMEOUT, UPDATED) contain only digits
(or set sensible defaults) before doing any arithmetic or comparisons; if any
required numeric value (especially COMPLETED or TOTAL) is empty or non-numeric,
log the raw STATE_OUTPUT/ERROR line and skip the math/stall checks (or treat
missing values as 0) to avoid "integer expression expected" failures. Insert
this guard immediately after the parsing block (where COMPLETED/TOTAL are
assigned) and before the CURRENT_TIME/TIME_DIFF and the if [ "$COMPLETED" -eq
... ] checks; use a POSIX-safe numeric test (e.g. a regex like ^[0-9]+$) against
COMPLETED and TOTAL, bail out of the numeric section or reset to 0 when
validation fails, and ensure RATE and TIME_DIFF computations only run when
TIME_DIFF > 0 and inputs are valid.
In `@CURRENT_RUN_STATUS.md`:
- Around line 33-35: The emphasized line "*Auto-updated every 15 minutes*" is
flagged as a pseudo‑heading by markdownlint MD036; replace the emphasis with
either a proper heading (e.g., "## Auto-updated every 15 minutes") or a plain
sentence ("Auto-updated every 15 minutes.") to remove the pseudo‑heading
formatting while keeping the same content; locate the line containing
"*Auto-updated every 15 minutes*" in CURRENT_RUN_STATUS.md and update it
accordingly.
In `@current_status_report.txt`:
- Line 3: Replace the literal "Report Time: $(date)" so the file contains a real
timestamp or an explicit placeholder; update the line "Report Time: $(date)" to
either a generated timestamp string (e.g., current ISO datetime) or a clear
placeholder like "Report Time: <generated_at>" so readers don’t see the
unevaluated $(date).
In `@enhanced_monitor.sh`:
- Line 12: Remove the insecure option StrictHostKeyChecking=no from the ssh
invocation and pre-seed the target host key into a known_hosts file before
opening the connection: run ssh-keyscan -t rsa,ecdsa,ed25519 "$VM_IP" and append
the output to a trusted known_hosts (for example "$HOME/.ssh/known_hosts" or a
dedicated file), ensure the file has correct permissions (chmod 600), then call
ssh with -o UserKnownHostsFile=path_to_known_hosts and keep -o ConnectTimeout=10
and the existing "$ADMIN_USER@$VM_IP" invocation (the here-doc and tee to
"$LOG_FILE" remain unchanged) so SSH host key verification stays enabled and you
still log output.
In `@MCP_TOOL_CALL_ANALYSIS.md`:
- Around line 5-8: Update the root-cause wording in MCP_TOOL_CALL_ANALYSIS.md so
it aligns with SUPERMODEL_MCP_INVESTIGATION.md: either change the claim that
"graph_status" failures are the primary root cause to a preliminary hypothesis
or revise it to reflect that the investigation found "summary" failing due to a
missing API key while "graph_status" usually succeeds; explicitly mention both
observations (graph_status failure rate and summary/API key finding) and add a
short sentence clarifying which claim is provisional to avoid mixed signals,
referencing the terms "graph_status" and "summary" and the investigation
document name SUPERMODEL_MCP_INVESTIGATION.md.
In `@MORNING_BRIEFING.md`:
- Around line 15-17: Replace the hardcoded IP in the ssh example ("ssh
azureuser@23.100.27.132" in MORNING_BRIEFING.md) with a reference to the .vm_ip
file or a <VM_IP> placeholder (e.g., use `ssh azureuser@$(cat .vm_ip)` or `ssh
azureuser@<VM_IP>`), and update any other docs that contain the same literal
(START_HERE.md, CURRENT_RUN_STATUS.md, BENCHMARK_RUNBOOK.md,
OVERNIGHT_SUMMARY.md, .claude_notes) to use the .vm_ip reference or placeholder
consistently; keep the tmux command (tmux attach -t mcpbr) unchanged.
In `@OVERNIGHT_SUMMARY.md`:
- Around line 119-125: Update the fenced code block containing the file list
(the block delimited by the triple backticks ``` that includes lines like
~/mcpbr-config.yaml, ~/comparison.log, ~/run-comparison.sh, ~/.mcpbr_state/) to
add a language tag by changing the opening fence to ```bash so the block is
explicitly marked as bash and satisfies Markdownlint MD040.
In `@SUPERMODEL_MCP_INVESTIGATION.md`:
- Around line 27-31: The document contains real API keys embedded in example
output (e.g., the SUPERMODEL_API_KEY environment variable showing values
starting with "smsk_live_"); remove or redact those sensitive values by
replacing them with a placeholder (e.g.,
SUPERMODEL_API_KEY='REDACTED_OR_ROTATED_KEY') in all occurrences (notably the
shown examples around the visible snippet and the referenced ranges 148-152 and
246-249), then rotate the exposed key immediately and coordinate git history
scrubbing to remove the leaked secret from the repository history.
🧹 Nitpick comments (1)
.mcpbr_state/evaluation_state.json (1)
1-521: Consider keeping run-state out of version control.This file looks like a live run artifact. Every new run will rewrite timestamps and task stats, creating huge, noisy diffs. A simple pattern is: add
.mcpbr_state/to.gitignore, and keep a small redacted sample indocs/if you need a reference snapshot.
Updates: - Upgraded @supermodeltools/sdk from 0.3.8 to 0.6.0 - Use SupermodelClient wrapper with automatic job polling - Simplified API calls - polling is now handled by SDK - Returns unwrapped results directly instead of async envelopes The SDK's SupermodelClient handles all polling logic internally, so MCP tools can use graph generation like synchronous APIs. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
7bb73eb to
eeb8314
Compare
Prevents evaluation, monitoring, and benchmark files from being accidentally committed to the repository.
Summary
Updates the MCP server to use the new 0.6.0 SDK with built-in async job polling support.
Changes
@supermodeltools/sdkfrom 0.3.8 to 0.6.0SupermodelClientwrapper instead of rawDefaultApiTechnical Details
The 0.6.0 SDK introduces a
SupermodelClientwrapper (seeasync.ts) that handles all polling logic internally:completedorfailedretryAfterhints from the API for efficient polling intervalsThis means MCP tools can now use graph generation like synchronous APIs - the client waits and returns the final result.
Before (0.3.8):
After (0.6.0):
Testing
npm run build)Closes #77
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.