fix: dedup dependency auto-apply to prevent duplicate tool mints (Gap #10)#23
Merged
dhruva-reddy merged 6 commits intomainfrom May 10, 2026
Merged
fix: dedup dependency auto-apply to prevent duplicate tool mints (Gap #10)#23dhruva-reddy merged 6 commits intomainfrom
dhruva-reddy merged 6 commits intomainfrom
Conversation
…10) Targeted assistant pushes minted duplicate dashboard tools when bootstrap pull stored an existing dashboard tool under a name-slugged state key (e.g. `end-call-67aea057`) instead of the user's original local key. The exact-key lookup in `ensureToolExists` / `ensureStructuredOutputExists` missed and POSTed a fresh duplicate. Each subsequent targeted push repeated the cycle, accumulating dashboard orphans. This adds a name-based dedup check between the exact-key short-circuit and the create path, in two layers: 1. State-side: scan state for any key whose `extractBaseSlug` matches the local payload's slugified name (handles bootstrap-renamed keys). 2. Dashboard-side: lazy-fetch the live `/tool` (and `/structured-output`) list once per push and check for a remote resource with the same canonical name. When >1 distinct UUID matches the same name (real on-dashboard duplicates from prior bug runs), pick the lex-smallest UUID for stable adoption, warn naming the loser UUIDs, and point at `npm run cleanup`. Never mint another duplicate. Adoption flow: - Re-key state to the adopted UUID under the local resourceId. - Drop other state keys pointing at the same UUID and mark them touched, so a subsequent full push doesn't orphan-delete the adopted dashboard resource (Stack J / mergeScoped flushes the deletion). - Route through `applyTool`/`applyStructuredOutput` so the local payload PATCHes the dashboard with the standard drift-check flow, instead of recording a fake `lastPushedHash` that would silently drop a locally-edited dependency. Tests: 12 unit tests for `findExistingResourceByName` covering state-only, dashboard-only, both-agree, ambiguous (state-vs-state, state-vs-dashboard), no-name, exact-key-excluded, no-match. All 114 suites pass. Refs: improvements.md §10
…CLAUDE.md - src/dep-dedup.ts: drop "Gap #10" issue marker from the file header (it rots; the rationale is what matters, not the tracker reference). - docs/learnings/tools.md: new section "Renaming a tool file is safe — the engine dedups by `function.name`" — explains the auto-apply dedup safety net, the 🔁 /⚠️ log line semantics, and the cleanup path. Counterpart in docs/learnings/structured-outputs.md cross-references it. - AGENTS.md: add `outbound-campaigns.md` to the Learnings & recipes table (was missing); refresh the docs/learnings/ tree in the Project Structure section to be complete; add an explicit "Where new knowledge goes" table pinning the convention (per-resource tips → docs/learnings/<topic>.md; engine-friction log → improvements.md; rationale → code comments; onboarding → README.md). - CLAUDE.md: sync the Required Reading Order list with AGENTS.md's table (was missing voice-providers, outbound-agents, outbound-campaigns, voicemail-detection); add a brief "Where new knowledge goes" reminder pointing back at AGENTS.md as the canonical convention table. No source behavior changes. Build clean, 114/114 tests pass.
…identifier refs from comments Code-review follow-ups on PR #23: - src/dep-dedup.ts: replace `Record<string, unknown>` + `as`-cast with a named `NameablePayload = { name?: unknown; function?: unknown }` shape and `in`-operator narrowing. No casts, no laundered types — the function reads two known paths and narrows them at use. - src/push.ts: scrub "Gap #10" / "Stack J" / "improvements.md #15" identifiers from comments. These were internal stack/log markers that don't help anyone reading the code; rephrased in domain language while keeping the rationale. Also drops redundant `as Record<string, unknown>` casts at call sites and reuses `extractResourceName` for the display-name fallback in dedup warnings. - src/push.ts: extend dedup to assistants. The squad → assistant auto-apply path (`ensureAssistantExists`) had the same bug class as tools / SOs — bootstrap pull stores assistants under `<slug>-<uuid8>` keys, and a squad referencing the original local key would mint a duplicate assistant on every push. Adds `getExistingRemoteAssistants` lazy-fetch + dedup branch with the same orphan-deletion guard and apply-via-PATCH flow already in place for tools / SOs. Documents in the DependencyContext comment why simulations / personalities / scenarios / sim-suites are NOT covered: they're not auto-applied as dependencies anywhere in the engine, so the bug class doesn't fire. - tests/dep-dedup.test.ts: add explicit assistant-payload test (top-level `name`, no nested `function`). Build clean, 115/115 tests pass (was 114, +1 new assistant test).
Two cleanup sweeps prompted by review feedback on PR #23: Sweep 1 — internal stack/log identifiers in code comments. References to "Stack F/G/H/I/J" and "improvements.md #N" are internal-only and mean nothing to a customer reading the code. Each comment is rephrased in domain language while preserving the WHY: - Stack F → "per-resource content-hash state schema" / "schema migration" - Stack G → "drift detection layer" - Stack H → "snapshot-on-push for rollback" - Stack I → "ETag-based optimistic concurrency" - Stack J → "scoped state writes" - improvements.md #N → dropped entirely (the rationale stands on its own) Touched: src/api.ts, cleanup.ts, dep-dedup.ts, drift.ts, pull.ts, push.ts, resolver.ts, sim-cmd.ts, sim.ts, snapshot.ts, state-merge.ts, state-serialize.ts, types.ts. Sweep 2 — customer-specific identifiers in docs/learnings. Customer brand names (`iForm`, `Mudflap`) and internal ticket IDs (`PRISM-481`, `PRISM-528`, `PRISM-474`) replaced with generic placeholders so the public template doesn't carry customer artifacts: - iForm → Acme Logistics (in scenario examples) - Mudflap → "a customer rollout" (in cross-references) - PRISM-* tickets → dropped entirely - handoffToiFormSales → handoffToAcmeSales - b2b-invoice-end-call.yml → intake-end-call.yml (in renaming example) Touched: docs/learnings/assistants.md, simulations.md, squads.md, tools.md, voice-providers.md. No source-behavior changes. Build clean, 115/115 tests pass.
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
1 participant
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
Fixes Gap #10 — targeted assistant pushes were minting duplicate dashboard tools when bootstrap pull stored an existing dashboard tool under a name-slugged state key (e.g.
end-call-67aea057) instead of the user's original local key (b2b-invoice-end-call). The exact-key lookup inensureToolExists/ensureStructuredOutputExistsmissed and POSTed a fresh duplicate; each subsequent targeted push repeated the cycle, accumulating dashboard orphans.What changed
src/dep-dedup.ts(new) — pure-logic helpers (slugify,extractBaseSlug,extractResourceName,findExistingResourceByName). Imports only./types.tsso it's testable without bootingconfig.ts's CLI parse.src/push.ts—ensureToolExistsandensureStructuredOutputExistsnow run a name-based dedup check between the exact-key short-circuit and the create path:extractBaseSlugmatches the local payload's slugified name (catches bootstrap-renamed keys)./tool(or/structured-output) list once per push, cached onDependencyContext, and check for a remote resource with the same canonical name.When >1 distinct UUID matches the same name (real on-dashboard duplicates from prior bug runs), pick the lex-smallest UUID for stable, deterministic adoption; warn naming the loser UUIDs; point at
npm run cleanup. Never mint another duplicate.Adoption flow handles two lifecycle traps surfaced in code review:
--force) doesn't see them as "tracked but no local file" and DELETE the dashboard resource we just adopted. (Stack J /mergeScopedflushes the deletion.)applyTool/applyStructuredOutput(which take the PATCH path becauseexistingUuidis now set), running the standardcheckDriftForUpdateflow and pushing the local payload. Avoids recording a fakelastPushedHashthat would silently drop locally-edited dependencies on the next push.tests/dep-dedup.test.ts(new, 15 tests) — coversfindExistingResourceByNamefor: state-only match, dashboard-only match, both-agree, ambiguous (state-vs-state and state-vs-dashboard), no-name payload, exact-key-excluded (caller's job), no-match, empty inputs, top-level-name-wins, remote payload using top-levelname.Test plan
npm run buildclean (tsc --noEmit)npm test114/114 pass (was 99 before; +15 new dep-dedup tests + 0 regressions)Skipped / non-blocking notes from review
getExistingRemoteToolsreturns[]in dry-run (no API call), so dry-run only exercises state-side dedup. Acceptable: dry-run is meant to be cheap; the state-side check covers the common case.slugify/extractBaseSlugdeliberately duplicated betweenpull.tsanddep-dedup.tsso the new module stays config-free for testability — file header documents the rationale.Improvements log
Will mark
improvements.md §10as[RESOLVED YYYY-MM-DD] (#<this-PR>)in a follow-up commit on this branch once the PR # is known.