feat(performance): add progressive disclosure tests and compositional routing acceptance#6341
Draft
ericksoa wants to merge 38 commits into
Draft
feat(performance): add progressive disclosure tests and compositional routing acceptance#6341ericksoa wants to merge 38 commits into
ericksoa wants to merge 38 commits into
Conversation
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
ericksoa
added
integration: openclaw
Contributor
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in the branch is 96%. Coverage data for the branch is not yet available. Show a code coverage summary of the most covered files.
TypeScript / code-coverage/cliThe overall coverage in the branch is 76%. Coverage data for the branch is not yet available. Show a code coverage summary of the most covered files.
Updated |
Contributor
|
🌿 Preview your docs: https://nvidia-preview-pr-6341.docs.buildwithfern.com/nemoclaw |
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds the progressive tool-disclosure benchmark stack: deterministic synthetic inputs, live execution and recording infrastructure, statistical reporting, frozen-evidence validation, CLI wiring, docs, navigation, and tests. ChangesProgressive Tool-Disclosure Benchmark
Estimated code review effort: 5 (Critical) | ~120 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
E2E Advisor RecommendationRequired E2E: Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
Contributor
E2E Target RecommendationRequired E2E targets: Dispatch required E2E targets:
Full E2E target advisor summaryE2E Target AdvisorBase: Required E2E targets
Optional E2E targets
Relevant changed files
|
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Contributor
PR Review Advisor — BlockedMerge posture: Do not merge until addressed Action checklist
Findings index
🚨 Required before mergeAddress these before merging unless a maintainer explicitly overrides the advisor with rationale.
|
Contributor
PR Review Advisor (Nemotron Ultra) — BlockedMerge posture: Do not merge until addressed Action checklist
Findings index
🚨 Required before mergeAddress these before merging unless a maintainer explicitly overrides the advisor with rationale.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
scripts/bench/tool-disclosure/catalog.ts (2)
242-249: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider
readonly SyntheticTool[]for consistency with the immutability posture elsewhere.
SyntheticCatalog.toolsis mutable while sibling contracts intypes.tsconsistently usereadonly X[]for frozen evidence data. Given this catalog is meant to be a deterministic, frozen artifact, markingtools(and theSyntheticTool[]return types ofgenerateSyntheticTools) asreadonlywould better signal and enforce immutability.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/bench/tool-disclosure/catalog.ts` around lines 242 - 249, SyntheticCatalog.tools is mutable while this catalog is intended to be a frozen deterministic artifact. Update the SyntheticCatalog interface to use readonly SyntheticTool[] for tools, and propagate the same readonly return type through generateSyntheticTools so its output matches the immutability pattern used elsewhere in types.ts and related contracts.
648-660: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueStatic-analysis regex flag is a false positive here, but worth a defensive note.
schema.patternonly ever originates from hardcoded literals in this file (resourceIdSchema,correlation_id), never from external input, so there's no current ReDoS exploit path. The patterns themselves are simple anchored character classes without nested quantifiers/backreferences.That said, recompiling the
RegExpon every validation call is wasted work given catalog sizes up to 2,209 tools, andvalidateAgainstSchemais a generic validator that could later be fed attacker-influenced patterns if reused elsewhere. Consider caching compiled patterns (e.g., aWeakMap/module-level cache keyed by pattern string) as a defensive/perf improvement.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/bench/tool-disclosure/catalog.ts` around lines 648 - 660, The string validation in validateAgainstSchema repeatedly recompiles schema.pattern with new RegExp on every call, which is unnecessary work and could become risky if reused with broader inputs later. Add a small compiled-regex cache in or near validateAgainstSchema, keyed by the pattern string, and reuse the cached RegExp when checking schema.pattern in the string branch. Keep the existing behavior for minLength, maxLength, and enum unchanged.Source: Linters/SAST tools
scripts/bench/tool-disclosure/statistics.ts (1)
237-282: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the frozen primary-task count
24into a named constant.The literal
24appears in six places here (paired-task checks for success, schema, and latency gates plus their reason strings). It silently encodes the frozen 24-task primary corpus. A single named constant (e.g.PRIMARY_PAIRED_TASK_COUNT) improves clarity and prevents these gates from silently drifting apart if the count ever changes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/bench/tool-disclosure/statistics.ts` around lines 237 - 282, Extract the hardcoded paired-task count used in the success, schema, and latency gate checks into a shared named constant in statistics.ts, and replace every direct 24 comparison and matching reason-string reference in the gate logic with that constant so the primary-task corpus size is defined in one place and the checks stay consistent.scripts/bench/tool-disclosure/execute.ts (1)
613-623: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMaterialized evidence files aren't refreshed when a campaign aborts.
materializeAttemptJournalonly runs after the fullscheduledloop completes (Line 618); if a run exhausts its setup retries and throws (Line 613-614), execution jumps straight tofinallywithout regeneratingraw-events.jsonl/runs.jsonlfor the runs already completed. The raw per-attempt data is safely persisted inattempt-journal.jsonl, so this isn't data loss, but it does make partial progress harder to inspect without re-running/resuming.Consider calling
materializeAttemptJournal(options.outputDir, journal)from thefinallyblock so the summary artifacts always reflect the latest state.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/bench/tool-disclosure/execute.ts` around lines 613 - 623, materializeAttemptJournal is only invoked after the scheduled loop finishes, so aborted campaigns skip refreshing the summary artifacts. Move or duplicate the call into the execute() finally path so raw-events.jsonl and runs.jsonl are regenerated even when a setup retry failure throws, while preserving the existing cleanup of tunnels, servers, and proxy.test/bench/tool-disclosure-execute.test.ts (1)
62-90: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider covering the non-trailing corruption path too.
Only the recoverable trailing-partial-write scenario is tested. Given
recoverAttemptJournalalso throws for corruption that isn't trailing (Line 59 in execute.ts), a companion case asserting that throw would round out coverage of this crash-recovery logic.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/bench/tool-disclosure-execute.test.ts` around lines 62 - 90, The crash-recovery tests only cover the trailing partial append case in recoverAttemptJournal, so add a companion test for non-trailing corruption that should fail. Use the existing recoverAttemptJournal and materializeAttemptJournal test setup in tool-disclosure-execute.test.ts, but write a malformed line in the middle of valid entries and assert the function throws instead of recovering. Keep the new case adjacent to the current recovery test so both the happy-path recovery and the non-trailing corruption behavior are covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/bench/tool-disclosure/assemble-run.ts`:
- Around line 79-105: The static-visibility handling in assembleRun is not
terminal for zero-token runs, which lets completed runs fall through into
gradeTaskRun and crash when options.task is absent. Update the
outcome/correctness branching in assembleRun so any static-visibility run with
initialSchemaTokens === 0 is handled before the grading branch and returns a
successful result without calling gradeTaskRun, using the existing
outcome/correctness paths and the related helpers failedCorrectness and
successfulStaticCorrectness to keep the flow consistent.
In `@scripts/bench/tool-disclosure/schedule.ts`:
- Around line 123-165: The schedule logic in buildToolDisclosureSchedule is
duplicating values that already have named constants, which risks drift if the
source constants change. Replace the hardcoded task-count checks and
catalog-size literals with the existing PRIMARY_TASK_COUNT, STRESS_TASK_COUNT,
and STATIC_CATALOG_SIZES values (or derived references) so the frozen schedule
stays aligned with the canonical definitions. Keep the behavior the same, but
ensure all size validation and pair construction in buildToolDisclosureSchedule
uses those shared symbols instead of repeated numeric literals.
In `@scripts/bench/tool-disclosure/subprocess.ts`:
- Around line 78-94: The SIGKILL fallback in the subprocess timeout flow leaves
hardStop active after the child finishes, so the later kill can target a reused
process-group id. Update the timeout handling around the child.once("close")
Promise in subprocess.ts to keep a reference to hardStop and clear it in the
same cleanup path as timer, so both timers are canceled once the child exits or
errors. Use the existing hardStop, timer, and child cleanup logic together to
prevent the delayed SIGKILL from firing after successful termination.
In `@test/bench/tool-disclosure-recorder.test.ts`:
- Around line 75-98: Update the test titles in tool-disclosure-recorder.test so
every describe/it string follows the required behavior-oriented format with a
trailing local issue reference suffix like "(`#1234`)". Adjust the title(s) in the
tool-disclosure recording proxy block to keep the same meaning while appending
the issue-ref suffix to each affected test name.
In `@test/bench/tool-disclosure-report.test.ts`:
- Around line 222-256: The two it(...) titles in the paired statistics test
block are missing the required local issue-ref suffix. Update the test names in
the pairedBootstrapDifference and buildComparisonCells cases to keep the same
behavior-oriented wording but append the trailing “(`#1234`)” to each title so
they match the test naming convention.
In `@test/bench/tool-disclosure-validation.test.ts`:
- Around line 241-262: Add the required local issue-reference suffix to the
affected test names in the tool-disclosure validation suite. Update the describe
block and the relevant it titles in tool-disclosure-validation.test.ts so they
end with the trailing issue marker format used by the test/ convention (for
example, the same `(`#1234`)` suffix), keeping the existing wording intact and
applying it consistently to all affected titles.
---
Nitpick comments:
In `@scripts/bench/tool-disclosure/catalog.ts`:
- Around line 242-249: SyntheticCatalog.tools is mutable while this catalog is
intended to be a frozen deterministic artifact. Update the SyntheticCatalog
interface to use readonly SyntheticTool[] for tools, and propagate the same
readonly return type through generateSyntheticTools so its output matches the
immutability pattern used elsewhere in types.ts and related contracts.
- Around line 648-660: The string validation in validateAgainstSchema repeatedly
recompiles schema.pattern with new RegExp on every call, which is unnecessary
work and could become risky if reused with broader inputs later. Add a small
compiled-regex cache in or near validateAgainstSchema, keyed by the pattern
string, and reuse the cached RegExp when checking schema.pattern in the string
branch. Keep the existing behavior for minLength, maxLength, and enum unchanged.
In `@scripts/bench/tool-disclosure/execute.ts`:
- Around line 613-623: materializeAttemptJournal is only invoked after the
scheduled loop finishes, so aborted campaigns skip refreshing the summary
artifacts. Move or duplicate the call into the execute() finally path so
raw-events.jsonl and runs.jsonl are regenerated even when a setup retry failure
throws, while preserving the existing cleanup of tunnels, servers, and proxy.
In `@scripts/bench/tool-disclosure/statistics.ts`:
- Around line 237-282: Extract the hardcoded paired-task count used in the
success, schema, and latency gate checks into a shared named constant in
statistics.ts, and replace every direct 24 comparison and matching reason-string
reference in the gate logic with that constant so the primary-task corpus size
is defined in one place and the checks stay consistent.
In `@test/bench/tool-disclosure-execute.test.ts`:
- Around line 62-90: The crash-recovery tests only cover the trailing partial
append case in recoverAttemptJournal, so add a companion test for non-trailing
corruption that should fail. Use the existing recoverAttemptJournal and
materializeAttemptJournal test setup in tool-disclosure-execute.test.ts, but
write a malformed line in the middle of valid entries and assert the function
throws instead of recovering. Keep the new case adjacent to the current recovery
test so both the happy-path recovery and the non-trailing corruption behavior
are covered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: cd9aa198-ba06-4772-9bbd-4a2e7488cdfb
📒 Files selected for processing (34)
docs/index.ymldocs/inference/progressive-tool-disclosure-benchmark.mdxpackage.jsonscripts/bench/README.mdscripts/bench/tool-disclosure/artifacts.tsscripts/bench/tool-disclosure/assemble-run.tsscripts/bench/tool-disclosure/catalog.tsscripts/bench/tool-disclosure/drivers.tsscripts/bench/tool-disclosure/execute.tsscripts/bench/tool-disclosure/grading.tsscripts/bench/tool-disclosure/mcp-server.tsscripts/bench/tool-disclosure/openclaw-fixture.tsscripts/bench/tool-disclosure/quick-tunnel.tsscripts/bench/tool-disclosure/recorder.tsscripts/bench/tool-disclosure/report.tsscripts/bench/tool-disclosure/run.tsscripts/bench/tool-disclosure/schedule.tsscripts/bench/tool-disclosure/statistics.tsscripts/bench/tool-disclosure/subprocess.tsscripts/bench/tool-disclosure/tasks.tsscripts/bench/tool-disclosure/telemetry.tsscripts/bench/tool-disclosure/types.tsscripts/bench/tool-disclosure/validation.tstest/bench/tool-disclosure-catalog.test.tstest/bench/tool-disclosure-drivers.test.tstest/bench/tool-disclosure-execute.test.tstest/bench/tool-disclosure-fixture.test.tstest/bench/tool-disclosure-mcp-server.test.tstest/bench/tool-disclosure-quick-tunnel.test.tstest/bench/tool-disclosure-recorder.test.tstest/bench/tool-disclosure-report.test.tstest/bench/tool-disclosure-schedule.test.tstest/bench/tool-disclosure-telemetry.test.tstest/bench/tool-disclosure-validation.test.ts
Comment on lines
+75
to
+98
| describe("tool-disclosure recording proxy", () => { | ||
| it("fails closed for non-loopback listeners and credential-bearing upstream URLs", () => { | ||
| expect(() => | ||
| createToolDisclosureRecordingProxy({ | ||
| upstreamBaseUrl: "https://inference.example/v1", | ||
| listenHost: "0.0.0.0", | ||
| }), | ||
| ).toThrow("must be exactly 127.0.0.1"); | ||
| expect(() => | ||
| createToolDisclosureRecordingProxy({ | ||
| upstreamBaseUrl: "https://user:password@inference.example/v1", | ||
| }), | ||
| ).toThrow("must not contain credentials"); | ||
| expect(() => | ||
| createToolDisclosureRecordingProxy({ | ||
| upstreamBaseUrl: "https://inference.example/v1?api_key=private-key", | ||
| }), | ||
| ).toThrow("must not contain a query or fragment"); | ||
| expect(() => | ||
| createToolDisclosureRecordingProxy({ | ||
| upstreamBaseUrl: "http://inference.example/v1", | ||
| }), | ||
| ).toThrow("plaintext upstreamBaseUrl is allowed only on loopback"); | ||
| }); |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Test titles are missing the required local issue-ref suffix.
The describe/it titles are behavior-oriented but omit the trailing (#1234) local issue ref (e.g. it("fails closed for non-loopback listeners ... (#1234)")). This applies to every it(...) title in this file.
As per coding guidelines: "Root-level integration tests under test/ should import source code, use ESM imports, and use behavior-oriented titles with local issue refs in a final (#1234) suffix."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/bench/tool-disclosure-recorder.test.ts` around lines 75 - 98, Update the
test titles in tool-disclosure-recorder.test so every describe/it string follows
the required behavior-oriented format with a trailing local issue reference
suffix like "(`#1234`)". Adjust the title(s) in the tool-disclosure recording
proxy block to keep the same meaning while appending the issue-ref suffix to
each affected test name.
Source: Coding guidelines
Comment on lines
+222
to
+256
| describe("tool-disclosure paired statistics", () => { | ||
| it("uses a deterministic 10,000-sample paired task bootstrap by default", () => { | ||
| const observations = [ | ||
| { direct: 10, progressive: 5 }, | ||
| { direct: 20, progressive: 16 }, | ||
| { direct: 30, progressive: 24 }, | ||
| ]; | ||
| const first = pairedBootstrapDifference(observations, { seed: 123 }); | ||
| const second = pairedBootstrapDifference(observations, { seed: 123 }); | ||
|
|
||
| expect(first).toEqual(second); | ||
| expect(first.bootstrap_samples).toBe(DEFAULT_BOOTSTRAP_SAMPLES); | ||
| expect(first.paired_tasks).toBe(3); | ||
| expect(first.estimate).toBe(-5); | ||
| expect(first.lower_95).toBeLessThanOrEqual(first.estimate); | ||
| expect(first.upper_95).toBeGreaterThanOrEqual(first.estimate); | ||
| }); | ||
|
|
||
| it("averages repetitions within a task before resampling task pairs", () => { | ||
| const runs = [ | ||
| makeRun({ mode: "direct", taskId: "task-a", repetition: 1, success: true }), | ||
| makeRun({ mode: "direct", taskId: "task-a", repetition: 2, success: false }), | ||
| makeRun({ mode: "progressive", taskId: "task-a", repetition: 1, success: true }), | ||
| makeRun({ mode: "progressive", taskId: "task-a", repetition: 2, success: true }), | ||
| makeRun({ mode: "direct", taskId: "task-b", success: true }), | ||
| makeRun({ mode: "progressive", taskId: "task-b", success: true }), | ||
| ]; | ||
| const [cell] = buildComparisonCells(runs, { samples: 100, seed: 1 }); | ||
|
|
||
| expect(cell.differences.success_percentage_points.paired_tasks).toBe(2); | ||
| expect(cell.differences.success_percentage_points.estimate).toBe(25); | ||
| expect(cell.direct.success_rate_percent).toBe(75); | ||
| expect(cell.progressive.success_rate_percent).toBe(100); | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- file: test/bench/tool-disclosure-report.test.ts ---\n'
cat -n test/bench/tool-disclosure-report.test.ts | sed -n '1,320p'
printf '\n--- suffix examples in test/**/*.test.* ---\n'
rg -n --glob 'test/**/*.test.{js,ts,mjs,mts,cjs,cts}' '\(#\d+\)$' test || true
printf '\n--- titles without suffix in test/**/*.test.* (sample matches) ---\n'
rg -n --glob 'test/**/*.test.{js,ts,mjs,mts,cjs,cts}' 'it\(|describe\(' test/bench test || trueRepository: NVIDIA/NemoClaw
Length of output: 244
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf -- '--- file: test/bench/tool-disclosure-report.test.ts ---\n'
cat -n test/bench/tool-disclosure-report.test.ts | sed -n '1,320p'
printf -- '\n--- suffix examples in test/**/*.test.* ---\n'
rg -n --glob 'test/**/*.test.{js,ts,mjs,mts,cjs,cts}' '\(#\d+\)$' test || true
printf -- '\n--- titles in test/**/*.test.* under test/bench ---\n'
rg -n 'it\(|describe\(' test/bench --glob '**/*.test.*' || trueRepository: NVIDIA/NemoClaw
Length of output: 27695
Add the local issue-ref suffix to these test titles. The it(...) names in this block follow the behavior-oriented style, but they still need the trailing (#1234) to match the test naming convention.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/bench/tool-disclosure-report.test.ts` around lines 222 - 256, The two
it(...) titles in the paired statistics test block are missing the required
local issue-ref suffix. Update the test names in the pairedBootstrapDifference
and buildComparisonCells cases to keep the same behavior-oriented wording but
append the trailing “(`#1234`)” to each title so they match the test naming
convention.
Source: Coding guidelines
Comment on lines
+241
to
+262
| describe("tool-disclosure complete-evidence validation", () => { | ||
| let fixture: CompleteEvidenceFixture; | ||
|
|
||
| beforeAll(() => { | ||
| fixture = buildCompleteEvidenceFixture(); | ||
| }); | ||
|
|
||
| it("rejects incomplete or mutable claim-grade manifest metadata", () => { | ||
| expect(() => validateFrozenManifest(fixture.manifest)).not.toThrow(); | ||
|
|
||
| const missingModelRevision = structuredClone(fixture.manifest); | ||
| missingModelRevision.inference.model_revision = ""; | ||
| expect(() => validateFrozenManifest(missingModelRevision)).toThrow(/claim-grade/u); | ||
|
|
||
| const mutableInferenceImage = structuredClone(fixture.manifest); | ||
| mutableInferenceImage.inference.container_digest = "fixture:latest"; | ||
| expect(() => validateFrozenManifest(mutableInferenceImage)).toThrow(/claim-grade/u); | ||
| }); | ||
|
|
||
| it("accepts the exact 1,884-run frozen matrix and rejects reordered or tampered evidence", { | ||
| timeout: 30_000, | ||
| }, () => { |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== file excerpt ==\n'
sed -n '220,290p' test/bench/tool-disclosure-validation.test.ts
printf '\n== issue-ref suffix occurrences in file ==\n'
rg -n '\(#\d+\)' test/bench/tool-disclosure-validation.test.ts || true
printf '\n== nearby test title patterns ==\n'
rg -n 'it\(".*#\d+\)|describe\(".*#\d+' test -g '*.test.ts' | head -n 50 || trueRepository: NVIDIA/NemoClaw
Length of output: 9050
Add the local issue-ref suffix to these test titles. test/bench/tool-disclosure-validation.test.ts:241-262 follows the root test/ convention, but the describe/it names are missing the required trailing (#1234) issue reference. Append the tracking issue if one exists.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/bench/tool-disclosure-validation.test.ts` around lines 241 - 262, Add
the required local issue-reference suffix to the affected test names in the
tool-disclosure validation suite. Update the describe block and the relevant it
titles in tool-disclosure-validation.test.ts so they end with the trailing issue
marker format used by the test/ convention (for example, the same `(`#1234`)`
suffix), keeping the existing wording intact and applying it consistently to all
affected titles.
Source: Coding guidelines
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
…benchmarks Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
…benchmarks Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Contributor
E2E Target Results —
|
| Job | Result |
|---|---|
| tool-disclosure-performance-smoke |
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
…benchmarks Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Contributor
E2E Target Results — ❌ Some jobs failedRun: 28881548680
|
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
…benchmarks Signed-off-by: Aaron Erickson <aerickson@nvidia.com> # Conflicts: # test/e2e/support/upload-e2e-artifacts-workflow-boundary.test.ts
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Contributor
E2E Target Results — ✅ All requested jobs passedRun: 28883408796
|
Contributor
E2E Target Results — ✅ All requested jobs passedRun: 28884388865
|
This was referenced Jul 7, 2026
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
This was referenced Jul 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds an opt-in, repeatable performance-test harness for direct versus progressive tool disclosure and an independent compositional tool-routing experiment. The full two-campaign procedure is hardware-neutral and records privacy-safe, statistically gated evidence on any compatible host. The routing experiment has a strict route-only acceptance command plus a separate claim-ineligible agent replay that verifies task correctness and reduced model-visible schemas.
This pull request remains a draft while maintainers review the implementation and evidence boundaries. No measured results are checked in, and no performance test runs automatically.
Related Issue
Related to #6251.
Changes
npm run performance:tool-disclosure -- prepare|execute|summarizewith a deterministic 2,209-tool catalog, frozen task sets, and a paired two-campaign schedule.route-acceptance, which evaluates a 20-tool, eight-case corpus spanning zero through five capabilities with a recorded decomposer and either portable or semantic embeddings.Type of Change
Quality Gates
Verification
npm run check:diffpassed at exact heada8a2f41cafter merging currentmain.npm run typecheckandnpm run typecheck:clipassed.npx vitest run --project integration test/performance/tool-disclosure-*.test.tspassed: 16 files, 138 tests.Live evidence
20a64a8c; 3/3 runtime scenarios passed.2ce14274; progressive and direct each completed the frozen task with one expected synthetic call. This was one fixed-order, claim-ineligible observation.559f268e; refinement improved exact decomposition from 5/8 to 7/8 cases and exact ordered forwarding from 4/8 to 7/8, but the run correctly failed because one five-step route did not complete.e0d1deb7; route-only acceptance passed, while the routed replay failed before the recorder because the gateway could not reach runner loopback. This led to the authenticated private-bridge path.a8a2f41c; all jobs passed. The route-only corpus completed 8/8 cases with no fallback and all refined strict gates at 1.0. Refinement improved exact decomposition from 0.75 to 1.0, exact tool recall at one from 0.944 to 1.0, and exact ordered selection/forwarding from 0.875 to 1.0. The routed replay completed the frozen task with one expected call, no fallback or bypass, and reduced model-visible schemas from 79 to 16 on both model requests. Progressive and direct frozen cells also succeeded with one expected call each. Provider restore, proxy shutdown, sandbox cleanup, and MCP cleanup all passed.The live smoke is operational acceptance evidence only. Its single fixed-order observations and portable lexical embedder do not support performance or universal quality claims; those require the documented full two-campaign procedure on a recorded hardware and software configuration.
Signed-off-by: Aaron Erickson aerickson@nvidia.com