Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

fix(desktop): land the 21 confirmed review findings on the v11 surface#643

Open
mattcfilbert wants to merge 7 commits into
feature/desktoptableau/tableau-mcp:feature/desktopfrom
claude/v11-fixestableau/tableau-mcp:claude/v11-fixesCopy head branch name to clipboard
Open

fix(desktop): land the 21 confirmed review findings on the v11 surface#643
mattcfilbert wants to merge 7 commits into
feature/desktoptableau/tableau-mcp:feature/desktopfrom
claude/v11-fixestableau/tableau-mcp:claude/v11-fixesCopy head branch name to clipboard

Conversation

@mattcfilbert

Copy link
Copy Markdown
Contributor

Stacked on #642. This lands the review round: 21 confirmed findings, the two pre-specified fixes (description-rule inversion, corpus cue pins), and the reviewer's DEC 3 recommendation, so #642 plus this branch is the reviewed surface.

The decisions this PR asks you to accept:

  1. Reuse is non-terminal. A duplicate-sheet hit returns reused / applied:false with a rebuild offer, consults the events anchor (user edited → real rebuild), and records no terminal recovery state. The dedupe stays; the "done" it used to mint from a name lookup is gone.
  2. The encodings report always ships. classify attaches it on every bind (empty arrays included), so a successful bind can no longer claim its encoding analysis never ran. This removes the undefined-means-two-things overload at the source.
  3. Parameter descriptions carry provenance, not tool names — surface-wide gate. Error text still names tools (the reachable-tools gate makes that safe); parameter descriptions never do. This inverts the origin rule fix(desktop): stop claiming what we cannot verify and offering what the caller cannot use #642 shipped, per the review's verdict.
  4. Mechanical fixes as reviewed: canonical-name focus at the seam, the timeout env var actually wired, activate-sheet enveloped + surface-wide invariant test, six stale mode=file strings, underscore-blind gate regex, %-heading decode crash, five-segment field regex, corpus cue pins + five honest test renames, ratcheted byte caps, AGENTS.md lockstep claim scoped to what the gate actually checks.

Also fixed while in the file: the calc-first route regex never matched the % spelling ("revenue growth %") — found by a live probe during a coverage audit, verified before fixing.

Open question for the merger (from the review, unresolved on purpose): #642 removes served parameters; repo policy says MAJOR, the bump was minor+patch. Bump or amend the policy — either way, not silently. This PR stays at patch (2.57.2) pending that call.

Lockstep: classify.ts and fieldReferenceRewriter.ts hashes regenerated; a2td re-sync is on the port-debt ledger.

Validation: full suite 6053 passed / 332 skipped (net +26 over #642's 6027), tsc clean, lint clean — run by the orchestrator, not taken from builder reports.

🤖 Generated with Claude Code

Base automatically changed from claude/v11-mcp to feature/desktop July 25, 2026 17:34
mattcfilbert and others added 7 commits July 25, 2026 13:38
…hips

The duplicate-sheet reuse path returned applied:true and a terminal done from a
name lookup. It now returns reused/applied:false with a rebuild offer, falls
through to a real rebuild when the events anchor shows the sheet changed since
it was built, carries authored_calcs, and no longer writes a terminal
bind-recovery record that would block the very rebuild it suggests.

The classifier now always attaches the encodings report (empty arrays included),
so a successful bind no longer claims the encoding analysis never ran. The guard
test asserts the real receipt string instead of one that exists nowhere.

Also: the unfilled-encoding steer names both calls (add-field then
apply-worksheet), the fifth derivation table is gone (both call sites use the
one throwing resolver), and the calc-first route regex now matches 'growth %'.

Co-Authored-By: Claude <noreply@anthropic.com>
…es them

Inverts the origin-description rule: parameter descriptions must not contain a
registered tool name (that coupling goes stale on rename and was already wrong —
three descriptions named list-available-fields as the producer of a path it
never returns). Descriptions now state the semantic source and, for optional
opaque params, what happens when omitted. The gate enforces this surface-wide.

Error text keeps naming tools — the reachable-tools gate makes that safe — so
the apply-* recovery messages now name get-worksheet-xml / read-cached-xml /
write-cached-xml instead of periphrasis.

Also: resolve-field's stale-cache guard no longer requires session (the
description tells agents to omit it), and the byte-budget grandfather caps for
add-field and inject-template ratchet down to their new smaller sizes.

Co-Authored-By: Claude <noreply@anthropic.com>
…out env var

The apply seams resolve an entity-encoded or NFD sheet name to its canonical
form but forwarded the caller's raw name to activation, so navigation silently
dropped. The seam now overrides focus.sheetName with the canonical name — fixed
once, not at nineteen call sites.

TABLEAU_DESKTOP_CALL_TIMEOUT_MS above 60s did nothing: production never passed
clientOptions. The session manager now wires config.desktopCallTimeoutMs into
the External API client.

activate-sheet's not-found error carried a bare structuredContent block with no
message and no next action; it now uses the same envelope as every other
producer, and a new source-scan test enforces that invariant surface-wide.

Co-Authored-By: Claude <noreply@anthropic.com>
…r has

Six live strings still told the agent to call apply-* with mode=file — a
parameter DEC 7 deleted and the served schemas now reject. They name the cache
path contract instead. The unreachable over-cap note and its green test are
deleted. One dialog warning spelled execute-tableau-command with underscores;
fixed, and the reachable-tools gate now catches underscore spellings too. The
search recommendation no longer ends at an XML route the default profile
cannot take.

Knowledge section reads no longer throw URIError on headings containing a bare
percent sign (a real shipped heading has one); the raw fragment is the fallback.

The field-reference regex anchors the role, so a five-segment column instance
can no longer silently rename a field to its derivation suffix. The corpus test
pins the eleven cue outcomes skips were hiding and renames five tests to claim
only what they prove.

Co-Authored-By: Claude <noreply@anthropic.com>
…iter; 2.57.2

The consumer repo (agent-to-tableau-desktop) must re-sync these two
lockstep-core files; logged on the port-debt ledger.

Co-Authored-By: Claude <noreply@anthropic.com>
… manual

Co-Authored-By: Claude <noreply@anthropic.com>
…he mocks

The re-review of the fix round confirmed four P1s and two P2s; all fixed:

- The reuse anchor is now captured AFTER the apply. It was stored pre-apply, so
  on live Desktop (where the apply itself advances the event sequence) every
  repeat looked user-edited and the dedupe would never have fired — the mocks
  hid it because their events never advance.
- Reuse that authored calcs reports applied:true; the workbook was written.
- The recovery gate admits any call naming target_worksheet — an explicit
  rebuild instruction is not a bare resubmit — and reuse no longer erases
  recovery state, so the repeat-bind breaker still engages on bare loops.
- The multi-encoding steer chains worksheetFile through consecutive add-field
  calls instead of restarting each from the live sheet.
- columnRef and fieldMapping descriptions state their provenance again (from
  field resolution, never invented) without naming tools; byte caps ratchet
  down to the new smaller sizes.
- The surface-wide session-never-required assertion is restored across the
  full registry.

Still unproven live (named in the PR): the post-apply anchor against a real
Desktop event stream; transports without getEvents store no anchor and the
receipt's presence-only caveat covers them.

Co-Authored-By: Claude <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

Morty Proxy This is a proxified and sanitized view of the page, visit original site.