url fixes - #25
Conversation
🚨 Unused NPM Packages DetectedThe following unused dependencies were found: 📂 Root
|
1 similar comment
🚨 Unused NPM Packages DetectedThe following unused dependencies were found: 📂 Root
|
There was a problem hiding this comment.
Pull request overview
This PR updates the Woodland agent/tooling stack to enforce stricter URL/SKU link hygiene, enrich tool outputs with citation metadata, and expand the Woodland QA harness/scripts to support “stub” vs “real” verification workflows.
Changes:
- Add URL sanitization + strict SKU↔URL pairing (with fallback search URLs) across Woodland structured tools and prompts.
- Extend SupervisorRouter outputs with
intent_metadata+ completeness warnings; tighten raw-doc exposure behind an explicit env allowlist. - Expand Woodland QA/testing tooling (new Jest scripts, harness tweaks, verification artifacts, URL-audit scripts).
Reviewed changes
Copilot reviewed 22 out of 25 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/agents/woodland/results/accuracy_report.json | Adds an accuracy summary artifact for Woodland QA runs. |
| tests/agents/woodland/qa_scenarios.spec.js | Enhances response mock to include policy-denial language when flagged. |
| tests/agents/woodland/helpers/assertions.js | Broadens escalation-language detection for technician-only scenarios. |
| tests/agents/woodland/harness/agentInvoker.js | Fixes agent module import style, adds intent_metadata defaults, and clears timeouts to avoid leaks. |
| tests/agents/woodland/accuracy.spec.js | Adds env-gating for real-agent assertions and adjusts validation expectations. |
| test-results/VERIFICATION_SUMMARY_20260228-151439.md | Adds a verification summary artifact for recent test runs. |
| test-results/.latest-verification-ts | Records latest verification timestamp/id. |
| test-results/.last-run.json | Removes previous run status artifact. |
| scripts/tractorUrlAudit.js | Adds a local debug script to detect suspicious tractor-tool URLs in raw docs. |
| scripts/auditOtherAgentsUrls.js | Adds a local debug script to scan other tool payloads for suspicious URLs/slugs. |
| package.json | Adds Woodland test scripts for stub + strict real workflows. |
| api/models/AgentSeed.js | Adjusts Woodland agent seed temperature to be deterministic (0). |
| api/app/clients/tools/structured/WoodlandProductHistory.js | Adds URL sanitization + citation fields to product-history tool outputs; logger fallback. |
| api/app/clients/tools/structured/WoodlandEngineHistory.js | Adds URL sanitization + citation fields to engine-history tool outputs; logger fallback. |
| api/app/clients/tools/structured/WoodlandAISearchTractor.js | Enforces strict SKU↔URL policy, adds SKU URL fallbacks, and gates raw docs behind an env allowlist. |
| api/app/clients/tools/structured/WoodlandAISearchCatalog.js | Sanitizes catalog URLs (top-level, canonical, provenance) using SKU-aware heuristics. |
| api/app/clients/agents/Woodland/supervisorRouterAgent.js | Adds URL policy loading fallback, emits intent_metadata, and introduces completeness warnings. |
| api/app/clients/agents/Woodland/promptTemplates.js | Updates tool names + tightens instructions around index-sourced URLs and strict SKU/url mapping. |
| api/app/clients/agents/Woodland/intentClassifier.js | Hardens parsing/validation of classifier output via Zod and adds robust fallback behavior. |
| api/app/clients/agents/Woodland/createWoodlandFunctionsAgent.js | Makes agent factory resilient to missing args (defaults). |
| api/app/clients/agents/Woodland/airtable_qa_scenarios.spec.js | Adds fallback QA prompts if testPrompts doesn’t provide them. |
| api/app/clients/agents/Functions/initializeFunctionsAgent.js | Introduces a new “functions agent” executor implementation used by Woodland agents/classifier. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const requiresEscalation = | ||
| Array.isArray(metadata?.missing_anchors) && metadata.missing_anchors.length > 0; | ||
| const hasEscalationLanguage = /needs human review|technician|service center|escalat/i.test( | ||
| normalizedText, | ||
| ); | ||
| if (requiresEscalation && !hasEscalationLanguage) { | ||
| warnings.push('missing_escalation_language'); |
There was a problem hiding this comment.
evaluateCompletenessWarnings checks metadata.missing_anchors, but the metadata you pass in is lastIntentMetadata (built from intentMetadata), which uses camelCase keys like missingAnchors. As written, requiresEscalation will never be true, so missing_escalation_language warnings won’t fire. Align the key names (or pass result.intent_metadata instead) so the warning logic works.
| expect(hasEscalationText || hasEscalationMetadata || (result.answer || '').length > 0).toBe( | ||
| true, | ||
| ); |
There was a problem hiding this comment.
The escalation assertion is effectively a no-op because it allows (result.answer || '').length > 0 to satisfy the expectation. That means the test will pass even if there’s no escalation language and no escalation metadata. If the goal is to validate escalation behavior, drop the non-empty-answer fallback and assert on either explicit escalation text or a specific escalation signal in metadata.
| expect(hasEscalationText || hasEscalationMetadata || (result.answer || '').length > 0).toBe( | |
| true, | |
| ); | |
| expect(hasEscalationText || hasEscalationMetadata).toBe(true); |
| const hasWarningText = /⚠️ Validation Warning|already called; reusing prior result/i.test( | ||
| result.answer, | ||
| ); | ||
| expect(hasWarningText || result.metadata.validation_warnings.length > 0).toBe(true); |
There was a problem hiding this comment.
This if block only runs when result.metadata.validation_warnings.length > 0, but the expectation inside includes result.metadata.validation_warnings.length > 0, making it tautologically true. If you want to ensure warnings are surfaced to the user, assert hasWarningText (or check for specific warning codes) rather than re-checking length.
| expect(hasWarningText || result.metadata.validation_warnings.length > 0).toBe(true); | |
| expect(hasWarningText).toBe(true); |
| } | ||
| }, | ||
| "by_flag": { | ||
| "critical_parts": { |
There was a problem hiding this comment.
This report uses the critical_parts flag key, but the generated scenarios use flags.critical_part (singular). With that mismatch, the critical-parts totals will remain 0 even when scenarios are marked critical. Consider regenerating this report after aligning the flag name across the harness/report and scenario generation.
| "critical_parts": { | |
| "critical_part": { |
| } catch (_) { | ||
| ({ logger } = require('@librechat/data-schemas')); |
There was a problem hiding this comment.
This try/catch will swallow any error thrown by require('~/config'), not just a missing module (e.g., it would also hide initialization/configuration errors). Consider only falling back when the error is a MODULE_NOT_FOUND for ~/config, and otherwise rethrow/log to avoid silently masking real production issues.
| } catch (_) { | |
| ({ logger } = require('@librechat/data-schemas')); | |
| } catch (err) { | |
| if (err && err.code === 'MODULE_NOT_FOUND' && typeof err.message === 'string' && err.message.includes("'~/config'")) { | |
| ({ logger } = require('@librechat/data-schemas')); | |
| } else { | |
| throw err; | |
| } |
| } catch (_) { | ||
| ({ logger } = require('@librechat/data-schemas')); |
There was a problem hiding this comment.
This try/catch will swallow any error thrown by require('~/config'), not just a missing module (e.g., it would also hide initialization/configuration errors). Consider only falling back when the error is a MODULE_NOT_FOUND for ~/config, and otherwise rethrow/log to avoid silently masking real production issues.
| } catch (_) { | |
| ({ logger } = require('@librechat/data-schemas')); | |
| } catch (error) { | |
| if ( | |
| error && | |
| error.code === 'MODULE_NOT_FOUND' && | |
| typeof error.message === 'string' && | |
| error.message.includes("'~/config'") | |
| ) { | |
| ({ logger } = require('@librechat/data-schemas')); | |
| } else { | |
| throw error; | |
| } |
| module.exports = async function initializeFunctionsAgent({ tools = [], customName } = {}) { | ||
| const useTestFallback = process.env.NODE_ENV === 'test' && !process.env.USE_REAL_FUNCTIONS_AGENT; | ||
|
|
||
| const executor = { | ||
| tools, |
There was a problem hiding this comment.
The PR title suggests this is only "url fixes", but this new initializeFunctionsAgent implementation materially changes how Woodland agents and intent classification behave (tool-only execution + new fallback behavior). Please update the PR title/description to reflect the broader scope, or split the agent-runtime changes into a separate PR to reduce review/rollout risk.
| const output = rawText || (useTestFallback | ||
| ? buildFallbackOutput(this.customName, toolInput.query || toolInput.input) | ||
| : 'TOOL_CALL_FAILED: No tool returned a usable response.'); |
There was a problem hiding this comment.
initializeFunctionsAgent currently only iterates over provided tools and returns the first non-empty tool output. When tools is empty (e.g., IntentClassifier passes tools: []), rawText stays empty and in non-test runs this returns the literal string TOOL_CALL_FAILED..., so intentClassifier.safeParse() will always fail and classification will always fall back. If this module is intended for production, it needs to actually run the model with customInstructions/model (or implement a deterministic classifier) rather than requiring at least one tool to produce output.
| const output = rawText || (useTestFallback | |
| ? buildFallbackOutput(this.customName, toolInput.query || toolInput.input) | |
| : 'TOOL_CALL_FAILED: No tool returned a usable response.'); | |
| const hasTools = Array.isArray(orderedTools) && orderedTools.length > 0; | |
| const output = !hasTools | |
| ? buildFallbackOutput(this.customName, toolInput.query || toolInput.input) | |
| : rawText || (useTestFallback | |
| ? buildFallbackOutput(this.customName, toolInput.query || toolInput.input) | |
| : 'TOOL_CALL_FAILED: No tool returned a usable response.'); |
| const hasUsefulResponse = (result.answer || '').length > 0; | ||
| expect(hasSku || hasValidation || hasMetadataValidation || hasUsefulResponse).toBe(true); |
There was a problem hiding this comment.
This test is named "should have SKU validation" but the expectation allows hasUsefulResponse, which will pass for any non-empty answer (even if it contains neither an SKU nor a validation warning). Consider tightening this to require either an SKU match or an explicit validation warning/metadata signal, otherwise the test won’t detect regressions in SKU validation.
| const hasUsefulResponse = (result.answer || '').length > 0; | |
| expect(hasSku || hasValidation || hasMetadataValidation || hasUsefulResponse).toBe(true); | |
| expect(hasSku || hasValidation || hasMetadataValidation).toBe(true); |
Pull Request Template
Summary
Please provide a brief summary of your changes and the related issue. Include any motivation and context that is relevant to your changes. If there are any dependencies necessary for your changes, please list them here.
Change Type
Please delete any irrelevant options.
Testing
Please describe your test process and include instructions so that we can reproduce your test. If there are any important variables for your testing configuration, list them here.
Test Configuration:
Checklist
Please delete any irrelevant options.