[pelikhan]

Motivation

Addresses code scanning alert 1054 (MEDIUM CWE-400). An authenticated MCP caller could supply an unknown tool or parameter identifier near the 1 MiB HTTP body limit, triggering expensive full Levenshtein matrices against registered names.

Changes

• Limit suggestion inputs and candidates to 128 UTF-16 code units before distance calculations, including a check after normalization.
• Share a 65,536 logical-cell work budget per error response, including across all unknown parameters, and reuse the existing two-row linear-memory distance helper.
• Preserve ordinary typo suggestions and valid registered identifiers above the suggestion limit. Oversized unknown identifiers retain standard HTTP/stdio errors with bounded display text and explicit length guidance.

The budget intentionally makes suggestions best-effort for large registries; it does not change tool dispatch or schema acceptance.

Validation

• 89 focused tests passed across MCP core and the shared distance helper.
• Regression tests cover lengths 127, 128, 129, near-1 MiB identifiers, normalization expansion, oversized candidates, and valid long registered names on HTTP and stdio.
• Measured recurrence counts verify zero distance work for oversized identifiers and a shared budget across a 200-name registry and multiple unknown parameters.
• Near-1 MiB tool and parameter requests returned bounded errors on both transports under a 32 MiB V8 heap.
• make fmt-cjs, make lint-cjs, npm run typecheck, and final make agent-report-progress passed. Four pre-existing unsafe-catch lint warnings remain.

[github-actions]

✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft.

Design Decision Gate 🏗️

Outcome: reported_no_action

The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible.

Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.

Generated from Design Decision Gate 🏗️ · routed: opus55 medium · ~11 AIC · ⌖ ~1 AIC · ◇ ~1 AIC · ⊞ 7.7K · ◷

🏗️ ADR gate enforced by Design Decision Gate 🏗️

[github-actions]

✅ PR Code Quality Reviewer completed the code quality review.

🔎 Code quality review by PR Code Quality Reviewer

[github-actions]

✅ Test Quality Sentinel completed test quality analysis.

Test Quality Sentinel

Outcome: reported_no_action

The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible.

Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.

Generated from Test Quality Sentinel · sonnet50 · ~12 AIC · ⌖ ~16 AIC · ◇ ~5 AIC · ⊞ 12.7K · ◷

🧪 Test quality analysis by Test Quality Sentinel

[github-actions]

🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer

[github-actions]

✅ Ponytail Reviewer completed successfully!

Ponytail Reviewer

Outcome: reported_no_action

The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible.

Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.

Generated from Ponytail Reviewer · gpt56 · ~8 AIC · ⌖ ~6 AIC · ⊞ 15.1K · ◷

Generated by Ponytail Reviewer for #73884

[github-actions]

Comment Memory

Peek at saved memory (pr-code-quality-reviewer)

reviewed_at: 2026-11-01T19:28:24Z
review_event: REQUEST_CHANGES
top_themes: maxSuggestions regression; unbounded registry scans for length-pruned candidates
files_reviewed: actions/setup/js/mcp_server_core.cjs, actions/setup/js/mcp_server_core.test.cjs
comment_count: 2

Note

This comment is managed by comment memory.

Expand the saved memory block to view or edit the persistent context for this thread.
Edit only the text inside the backtick fences; workflow metadata and the footer are regenerated automatically.

Learn more about comment memory

🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt50 · ~4 AIC · ⌖ ~1 AIC · ⊞ 1.7K · ◷
_{Comment /review to run again}

[github-actions]

Request changes

The resource-bound suggestion path still has two correctness/performance regressions: findSimilarTools ignores its public maxSuggestions argument, and the shared cell budget does not cap candidate scans that are rejected before distance calculation. These need to be fixed before merge.

Details

• Restore propagation of maxSuggestions; the existing helper contract is observable and currently returns up to three results regardless of the caller.
• Account for length-pruned candidates or otherwise bound per-unknown-parameter registry traversal; many short unknown keys can still trigger repeated full scans.

🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt50 · ~4 AIC · ⌖ ~1 AIC · ⊞ 1.7K
_{Comment /review to run again}

[github-actions]

Skills-Based Review 🧠

Applied /diagnosing-bugs and /tdd (triage classified this as a bug-fix). Requesting changes: the new helper is not wired into the two deployment paths that ship mcp_server_core.cjs.

📋 Blocking: missing `levenshtein_distance.cjs` in deployment

• actions/setup/setup.sh MCP_SCRIPTS_FILES (L244) does not include levenshtein_distance.cjs. mcp_server_core.cjs now requires it, so the mcp-scripts bundle fails at load time with Cannot find module "./levenshtein_distance.cjs". I reproduced this by running the real deployment block into a temp dir. SAFE_OUTPUTS_FILES (L495) already includes it, so only the mcp-scripts path is broken. The transitive-closure test in setup_sh_file_lists.test.cjs should catch this; I could not run it locally because vitest is not installed here.
• actions/setup/session_parsers.go L68 (go/redacted):embed lists mcp_server_core.cjs but not levenshtein_distance.cjs. pkg/cli/session_parser.go writes every embedded file to a temp dir, so the same missing dependency applies there.

Fix: add "levenshtein_distance.cjs" to MCP_SCRIPTS_FILES, and js/levenshtein_distance.cjs to the embed.

🔍 Smaller items (see inline comments)

• Not-found error duplicates and mangles the truncation suffix (handleMessage, ~L949). Verified at ~437 chars for a 300-char name.
• formatUnknownIdentifier can split a surrogate pair at the 128-unit boundary, producing ill-formed UTF-16.

✅ Positive highlights

• The budget arithmetic is correct. I checked it in plain Node: 16,384 cells for one 128-char comparison and 49,152 for a 200-name registry, matching the PR's stated limits.
• Shared two-row Levenshtein helper keeps memory linear, and issue_title_dedup.cjs now reuses it.
• Bounds are applied before the O(n·m) work, so oversized input is rejected cheaply.

⚠️ Limits of this review

• Test suite not executed locally (vitest missing). The PR's "89 tests passed" claim is unverified here.
• The budget tests spy on Math.min as a proxy for matrix work. That is fragile; a direct cell counter would be more robust.

@copilot please address the review comments above.

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · routed: haiku55 xhigh · ~9 AIC · ⌖ ~1 AIC · ⊞ 5.4K
_{Comment /matt to run again}

[Copilot]

🟡 Changes recommended

The new dependency is missing from both source inventories, causing deployed server startup failures, while candidate filtering allows repeated registry scans to bypass the shared budget.

2 open issues

• Add the new dependency to both deployed source inventories · New
• Charge the shared budget for every candidate visit · New

What changed in this PR

This PR reduces resource use in gh-aw’s MCP identifier suggestions while preserving valid registered tool and parameter names above the suggestion limit.

Changes:

• Adds 128-code-unit identifier limits and a shared 65,536-cell suggestion budget.
• Reuses the linear-memory Levenshtein helper and bounds oversized identifiers in error messages.
• Adds HTTP and stdio regression tests for length boundaries, shared budgets, and valid long identifiers.

File	Description
actions/​setup/​js/​mcp_server_core.test.cjs	Tests suggestion limits, bounded errors, and long-name compatibility.
actions/​setup/​js/​mcp_server_core.cjs	Adds bounded suggestion work and identifier display formatting.

_{Review effort: Max (auto)}

[pelikhan]

Addressed the review batch in 05f4588. All four inline comments have replies and are resolved; maxSuggestions already propagated correctly, and its exact-result tests are now stronger.

Also accepted the deployment blocker from the skills review: levenshtein_distance.cjs is now included in both MCP_SCRIPTS_FILES and the embedded session-parser sources. Existing shell deployment and embedded dependency-closure checks pass.

Validation: 124 focused JavaScript tests passed, typecheck passed, and make fmt plus the final make agent-report-progress passed, including impacted Go tests. Four pre-existing unsafe-catch lint warnings remain; no validation blockers.

Ok

[pelikhan]

Regarding the design gate notification: acknowledged as informational, with no actionable design change requested. The no-action status is not validation evidence; implementation checks were performed separately.

[pelikhan]

Regarding the code quality completion notification: accepted as a status update, with no additional change requested. Its actionable review findings were addressed in 05f4588.

[pelikhan]

Regarding the test quality notification: accepted as informational; no test change is requested here. Validation comes from the passing focused tests, typecheck, and repository gate, not the no-action status.

[pelikhan]

Regarding the skills review completion notification: accepted as a status update, with no new code change required. The substantive deployment and error-formatting findings were addressed in 05f4588.

[pelikhan]

Regarding the Ponytail review notification: accepted as informational, with no actionable finding to implement. The no-action status alone is not treated as verification.

[pelikhan]

Regarding the saved review snapshot: the historical findings are addressed in 05f4588 through exact maxSuggestions tests and candidate charging before filters. No further code change is needed, and the reviewer-managed memory is left unchanged.

[pelikhan]

Regarding the previous fix report: accepted as the existing verified state, with no new change required. This pass reverified deployment closure and the original scan-budget case, and answered the remaining Copilot dependency thread.