refactor(agent-bundle): dedupe route, JSON, and IP-range helpers onto shared owners - #660
Merged
Merged
Conversation
🦋 Changeset detectedLatest commit: 9af572b The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
ScriptedAlchemy
force-pushed
the
refactor/ponytail-dedupe-helpers
branch
from
September 6, 2026 00:38
a512673 to
c4b2393
Compare
commit: |
ScriptedAlchemy
marked this pull request as ready for review
September 6, 2026 00:45
ScriptedAlchemy
enabled auto-merge (squash)
September 6, 2026 00:45
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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
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
Consolidation only: local copies of helpers move onto their shared owners (
dev/http.ts,core/strict-json.ts,core/errors.ts,core/paths.ts) or the stdlib. Avoid-listed files (install/**,adapters/**,build/**exceptentry-exports.ts,dev/routes/**,dev/mcp-session/**,dev/mcp-apps/mcp-app-routes.ts,foreground-server.ts,runtime-*-routes.ts, …) were not touched.core/strict-json.ts— done.isRecordinclaude-plugin-validation,cursor-plugin-validation,portable-plugin-validation,mcp-tasks; prototype-checkingisRecord/isPlainRecord→isPlainRecordinmcp-app-bridge,mcp-app-host-profiles(also itsisConfigExtensionRecordalias),notice-retention,mcp-app-metadata. Note:isPlainRecordalso admits null-prototype objects, which the three=== Object.prototypecopies did not; every caller reads structured-clone orJSON.parseoutput, where such objects cannot occur.dev/http.ts— done. New exportsresponseJsonOrDestroy(response, body, status = 200)(the ninewriteJsonResponse(…, { destroyIfEnded: true })wrappers),badRequest(code, message): () => never(theinvalidShape/pathError/invalidRequestthrowers), andnoQuery(requestTarget, invalid). Applied toinspector-routes,logs/dev-log-routes,artifacts/artifact-routes,playground/{playground,hook-playground,lifecycle-replay,mcp-probe,host-discovery}-routes,eval/eval-routes. Ineval-routes, the inlinedecodedSegment,jsonBody,isRecordretype andhasOnlyalias are nowdecodedOpaqueSegment,readJsonBody, andhasOnlyfromdev/http.ts; codes AB8070/AB8072/AB8009/AB8001/AB8010 are unchanged.dev/routes/route-manifest-routes.tsskipped (avoid-list);web-host-routes.tshad no wrapper to replace.mcp-app-bridgeandmcp-app-binding-servicedropisJsonValue/cloneJson/jsonRecordforsnapshotMcpAppJson/cloneMcpAppJson/snapshotMcpAppJsonRecord/requireMcpAppJsonfrom the existingdev/mcp-apps/mcp-app-json.ts(which wrapssnapshotStrictJsonValue); one-line aliases keep the ~70 bridge call sites unchanged. The shared walker additionally rejects cyclic values (the old walkers recursed without bound) and accessor/non-enumerable/symbol properties; inputs arrive via structured clone orJSON.parse, where neither occurs. Error message${label} must be a finite JSON value.preserved.snapshotStrictJsonValue+mapStrictJsonReasonforplayground-store,runtime-mcp-registry,playground-routes— skipped, all three, per the "messages must be reproduced exactly" rule:playground-store.jsonandruntime-mcp-registry.finiteJsonemit a distinct… must not contain accessors.message thatStrictJsonReasoncannot distinguish fromnot-json, and both sort object keys (Object.keys(...).sort()) and the store emits null-prototype objects, whichsnapshotStrictJsonValuedoes not reproduce (observable in persisted JSON byte order).playground-routes.jsonValueenforces a depth bound (maxValueDepth = 32, AB8042) the shared walker has no equivalent for.combineSignals/CombinedAbortSignalinruntime-mcp-registry→AbortSignal.any(two call sites,dispose()plumbing removed).throwIfAborted(signal, label)inmcp-app-binding-service→signal?.throwIfAborted(): no test asserts the label, and every productionabort()on those signals passes anErrorreason (or the defaultAbortError), so the thrown value is identical.hasOwnone-liners inmcp-app-bridge,mcp-app-sandbox,mcp-app-metadata,app/index.ts→Object.hasOwn.pathExistsinepoch-store,cursor-plugin-validation,test/packed.ts→existsfromcore/paths.ts(identical:lstat+ENOENT).errorMessage— done inconfig/{command,rule,skill,dev-contracts,render-markdown}.ts,install-entry.ts,mcp-tasks.ts,mcp-server-runtime.ts, including both privatedescribeErrorcopies (mcp-tasks,mcp-server-runtime).parseOperatorEnv→util.parseEnv— skipped, not applied.util.parseEnv(Node 22.23) fails both existinglaunch-env.test.tsvectors: it truncates a double-quoted value at the first escaped\"(DOUBLE="two\nlines \"quoted\""→two\nlines \,ESCAPED="a \" b"→a \), accepts9BAD=ignoredas a key, and readsTRAILER="x" not-a-commentasxinstead of the dotenv literal"x" not-a-comment.launch-env.tsis untouched.build/entry-exports.ts→ TypeScript parser — done.scanEntryExportsSourcenow walksts.createSourceFile(...).statements(ExportAssignmentexcludingexport =, non-type-onlyExportDeclarationnamed elements,export defaultmodifier,export function main/export const|let|var … main). The 100-linestripCommentsAndStringstokenizer is deleted.scanEntryExportsSource(source, fileName?)lets TypeScript pick the grammar from the extension (.tsx/.jsxparse JSX,.tskeeps angle-bracket assertions); every caller passes the real path.export declare …statements are skipped since they emit nothing. One test edit:entry-shell.test.tsimportedstripCommentsAndStringsfor one assertion (toContain('export const main')after stripping a division expression); it is now the equivalent public-API assertionscanEntryExportsSource('const division = a / b / c; export const main = 1;').hasMainExport === true. Keeping the tokenizer alive only for that line would have left a test-only module in production, so I judged this the intended outcome; revert is one line if not.net.BlockList— done, new leafcore/special-ip.ts(isSpecialPurposeIp,isNonGlobalUnicastIpv6) imported bymcp-app-host-profilesandmcp-app-sandbox; both hand parsers deleted. The two old detectors did not share one table. Verified with a /tmp old-vs-new script over every hostname in the two test files plus the eleven requested vectors: identical for both callers except::ffff:127.0.0.1(dotted-quad mapped form), which both old parsers failed to parse and therefore treated as public;BlockListrejects it. That input is unreachable in production (WHATWGURLserialises it as[::ffff:7f00:1], which both old and new reject). Beyond the requested vectors, the sandbox's hand-rolled IPv4 table was an approximation of the IANA registry and now follows it exactly:192.31.196/24,192.52.193/24,192.175.48/24are newly rejected; ordinary public space in192.0/16,192.2/16,192.88/16,198.51/16outside the registry blocks (e.g.192.0.3.1,192.2.0.1,192.88.1.1,198.51.1.1) is newly accepted. The sandbox's documented fail-closed IPv6 rule (only2000::/3) is preserved viaisNonGlobalUnicastIpv6. Host-profiles behaviour is unchanged on every vector.rsc-runtimenotices ledger →recipientSchema— skipped. The ledger'srecipient()rejects whitespace-only axes (nonEmptyTexttrims) andnotices-ledger.test.tsassertsrecipient: { conversation: ' ' }→invalid-input;recipientSchemaisz.string().min(1)and accepts it, and.strict()rejects unknown keys the ledger silently drops. Reusing the journal schema would change the publish contract;@agent-bundle/runtimeis untouched, so the changeset names onlyagent-bundle.Validation
pnpm build && pnpm typecheck && pnpm lint && pnpm test:unit— green (4237 unit tests).artifact-routes,claude/cursor/portable-plugin-validation,command-config,dev-log-routes,entry-shell,epoch-store,eval-routes,hook-playground-routes,host-discovery-routes,inspector-routes,lifecycle-replay-routes,mcp-app-{binding-service,bridge,bridge-cancellation,host-profiles,metadata,preview-service,sandbox},mcp-probe-routes,mcp-server-runtime,mcp-tasks,notice-retention-config,playground-routes,rule-config,runtime-mcp-registry,skill-ir— 443 passed.git grep -l special-ip -- ':!repos'→ the two importers; no remaining references tostripCommentsAndStrings,combineSignals,parseIpv6, orspecialIpv4Prefixes.Deslop
Deslop: Claude Fable 5.1, 3 edits (dropped redundant type annotations on the bridge's
cloneJson/jsonRecordaliases; repaired anObject.Object.hasOwnsed artefact inside the sandbox's embedded script strings; removed a now-unusedlstatimport).Self-review
Reviewer:
gpt-5.6-sol-medium(generalPurpose; TraceDecay daemon was down sochange-risk-reviewercould not run). Scope: all 35 files, with emphasis on the sandbox IP policy (core/special-ip.ts), abort/throwIfAbortedsemantics, route status/body shape, null-prototype admission perisRecordcall site, and the entry-export scanner. Two findings, both fixed:build/entry-exports.tsparsed every entry as plain TS, so JSX before an export in a.tsxentry mis-parsed and JSX text containingexport defaultcould false-positive. Fixed — the source file name now selects the grammar;scanEntryExportsand bothconfig/validate.tscallers pass the real path. Tests added.export declare const|function maincounted as a runtimemainexport. Fixed — statements with adeclaremodifier are skipped. Tests added.The IP policy, abort, route, record-guard, and
errorMessagesubstitutions were verified to preserve the reviewed contracts.Second pass (same reviewer): no merge risks found.