Skip to content

fix(github): wire the block fields whose ids do not match their tool params - #7287

Open
waleedlatif1 wants to merge 7 commits into
stagingfrom
fix/github-block-param-wiring-v2
Open

fix(github): wire the block fields whose ids do not match their tool params#7287
waleedlatif1 wants to merge 7 commits into
stagingfrom
fix/github-block-param-wiring-v2

Conversation

@waleedlatif1

@waleedlatif1 waleedlatif1 commented Aug 30, 2026

Copy link
Copy Markdown
Collaborator

Summary

Eleven fields in the GitHub block render, accept input, and then arrive at their tool under a name the tool never reads. Three operations are unusable from the canvas as a result; the rest silently discard what the user typed.

The block's subBlock ids are the wiring contract — the serializer keys values by subBlock.id (serializer/index.ts:521-612), and GitHubBlock.tools.config declared only tool:, with no params mapper and no canonicalParamId anywhere in the file. So a subBlock named reaction_content can never populate a tool param named content.

Broken outright — the call dies before the HTTP request

validateRequiredParametersAfterMerge (tools/utils.ts:111-138) throws for any missing required user-or-llm param, so these three fail with an error naming a field that does not exist in the UI:

operation tool param block field user sees
Create issue reaction content reaction_content Content is required for GitHub Create Issue Reaction
Create comment reaction content reaction_content Content is required for GitHub Create Comment Reaction
Create milestone title milestone_title Title is required for GitHub Create Milestone

Silently inert — no error, the input is just dropped

gist_publicpublic (every gist created secret regardless of the selection), fork_namename, fork_sortsort, milestone_title/milestone_descriptiontitle/description on update, milestone_statestate, milestone_sortsort.

The fix

One tools.config.params mapper, driven by an alias table where each entry names the operations whose tool actually declares its target. GitHubV2Block already forwards config.params (github.ts:2433), so this fixes both block versions in one place.

Every assignment is guarded, and that is load-bearing rather than defensive. generic-handler.ts:189-191 merges { ...inputs, ...params(inputs) }, and providers/utils.ts installs this same function as the provider paramsTransform, spreading its result over the model's tool-call arguments. An unconditional write would clobber a model-supplied content/title/sort with undefined — and the agent path is the one path these fields work on today, because an LLM supplies params by their real names. A test asserts a model-supplied value survives the mapper untouched.

Every alias is scoped to its operations, and that scoping is load-bearing. shouldSerializeSubBlock (serializer/index.ts:91-93) serializes a non-empty mode: 'advanced' field without evaluating its condition, and seven of the eight sources are advanced. So a milestone_title left behind from Create Milestone is still in params after the user switches to Update PR — an unscoped alias would rewrite it to title and overwrite the PR title the user typed. Scoping also subsumes the collision problem: sort has two sources, and title/description/state share names with other operations' fields.

gist_public also needs coercion, not just a rename: it is a dropdown, so it stores the string 'true'/'false' while the tool declares public as a boolean, and the generic handler only JSON-parses json/array-typed inputs (generic-handler.ts:193-200). This follows the established pattern in blocks/blocks/ashby.ts:1083 and blocks/blocks/agentmail.ts:590. Presence is tested rather than truthiness so a deliberate false is not mistaken for an unset field — the block declares this input as boolean, so a writer following that schema stores false rather than the dropdown's 'false', and a truthy check would silently drop one of them.

Why not canonicalParamId

It looks like the declarative fix and it is the wrong tool here. buildCanonicalIndex (lib/workflows/subblocks/visibility.ts:98-125) groups by canonical id, so two fields renaming to the same target collapse into one group — only the last becomes basicId, and the other's value becomes invisible to the collapse at serializer/index.ts:589-610. fork_sort and milestone_sort both target sort, so exactly that would have happened and the fix would have silently not worked for forks. Verified with a probe against the real functions before choosing the mapper.

Renaming the subBlock ids to match was the other candidate, and it would orphan every saved workflow that holds a value under the old id.

Scope

Deliberately one mechanism: a block field whose id does not match its tool param. A comprehensive audit of this integration turned up several other defect classes, each left for its own PR because they are unrelated mechanisms and bundling them is what makes a diff unreviewable:

  • github_update_branch_protectionrestrictions has no subBlock at all, three required params are mode: 'advanced' and unmarked, and two need JSON/boolean coercion. That operation needs redesigning, not rewiring.
  • Pagination — the block has exactly one page subBlock, conditioned on github_list_tags. Twenty-two other list tools declare page and can never receive it.
  • Condition-array gaps — sort covers 1 of 5 search ops, per_page misses 3 ops, labels/assignees miss update_issue, draft/prerelease miss update_release.
  • github_check_star cannot return starred: false — GitHub signals not-starred with 404 and the shared executor throws at tools/index.ts:2838 before transformResponse. Fixing it means either an internal-operation conversion or a shared-executor change affecting ~10 other tools' dead branches.
  • workflow_id renders for github_list_workflow_runs, which declares no such param.
  • operation never reaches a block's params mapper on the agent tool-calling path. providers/utils.ts spreads it in for tool selection (:736-739) but builds the transform's input from block.params alone (:776), so every alias here skips on that path — the same no-op as before this mapper existed, since a model already supplies these params by their real names. All 91 blocks whose mapper branches on params.operation are equally affected, so closing it is a one-line provider-layer change that deserves its own blast-radius review.

Backwards compatibility

Purely additive. Zero subBlock ids added, removed, or renamed; zero required flips; zero visibility changes — the diff adds a mapper and a test file and touches nothing else. check-block-registry.ts origin/staging passes the subblock-ID stability check.

Nothing that currently succeeds starts failing, and no saved state is orphaned. But this is not a no-op, and the distinction is worth stating precisely:

  • Three operations go from throwing to working. Create Issue Reaction, Create Comment Reaction and Create Milestone raise <Field> is required today, so nothing can depend on their current behavior. Strictly better, zero risk.
  • Eight fields go from ignored to applied. A workflow that filled one of them will now behave differently — in the direction the user configured it. That is the point of the fix, but it is a real change, not a silent equivalence.

The one to read carefully is gist visibility. Selecting "Public" on Create Gist currently produces a secret gist; after this change it produces a public one. Every other alias moves data the user typed into a field that ignored it; this one changes who can see a resource. The three outcomes are pinned by tests:

Dropdown state Serialized Before After
never opened absent secret secret (unchanged)
"Secret" 'false' secret secret (unchanged)
"Public" 'true' secret public (changed)

Only an explicit "Public" selection changes, and it changes to what the field says it does. An untouched advanced dropdown is not serialized at all (isNonEmptyValue is false for nullish/empty), so the default path is untouched.

A stale list filter also cannot leak into a write: milestone_state is scoped to github_list_milestones only, so a leftover value cannot set the state of a milestone being created.

The agent tool-calling path is unaffected — guarded assignment leaves model-supplied values alone, and every alias skips there anyway.

Testing

blocks/blocks/github.param-wiring.test.ts — 84 tests. 194 pass across blocks/blocks/github and tools/github.

Covers: every rename reaching its target; every target being a param the tool really declares (asserted against the live registry, not a hardcoded list); the source id not being a tool param; guarded assignment never writing undefined; a model-supplied value surviving; ''/null/undefined treated as not-provided; gist_public coercion including the dropdown's real option ids; that sources sharing a target are condition-disjoint; and that V2 forwards the same mapper.

Also covers the stale-value regression directly: a leftover milestone_title must not become github_update_pr's title and must not clobber a title the user typed, swept across all seven advanced sources against unrelated operations; and an absent, undefined, empty or non-string operation emits nothing while leaving model-supplied values intact.

Verified red-first at every step: removing the mapper turns 31 red, removing the operation guard turns 10 red, and restoring the truthy gist_public check turns 2 red.

bun run lint, bun run type-check (no GitHub diagnostics), bun run check:audits (39 audits), and check-block-registry.ts origin/staging all pass. tool-metadata:generate and generate-docs produce no drift.

Type of Change

  • Bug fix

Checklist

  • Code follows project style guidelines
  • Self-reviewed my changes
  • Tests added/updated and passing
  • No new warnings introduced
  • I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

@vercel

vercel Bot commented Aug 30, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
docs Skipped Skipped Aug 30, 2026 8:58pm

Request Review

@greptile-apps

greptile-apps Bot commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR adds operation-scoped parameter aliases so GitHub block fields whose IDs differ from tool parameter names are forwarded correctly while preserving model-supplied values.

  • Maps reaction, milestone, fork, and gist fields to their corresponding tool parameters.
  • Coerces gist visibility to a boolean and prevents stale advanced fields from leaking across operations.
  • Adds focused coverage for alias validity, operation scoping, unset values, collisions, V2 forwarding, and gist visibility.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
apps/sim/blocks/blocks/github.ts Adds guarded, operation-scoped aliases and gist boolean coercion without leaving the previously reported documentation violation.
apps/sim/blocks/blocks/github.param-wiring.test.ts Adds comprehensive parameter-wiring regression coverage and now uses the required TSDoc comment form.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  A[Serialized block parameters] --> B[Operation-scoped alias mapper]
  B --> C{Source value set?}
  C -- No --> D[Leave tool parameters unchanged]
  C -- Yes --> E{Boolean alias?}
  E -- No --> F[Copy value to canonical tool parameter]
  E -- Yes --> G[Coerce gist visibility to boolean]
  F --> H[GitHub tool invocation]
  G --> H
  D --> H
Loading

Reviews (7): Last reviewed commit: "test(github): exercise the isSet guard i..." | Re-trigger Greptile

Comment thread apps/sim/blocks/blocks/github.param-wiring.test.ts Outdated

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 2 files

Confidence score: 5/5

  • Automated review surfaced no issues in the provided summaries.
  • No files require special attention.

Re-trigger cubic

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cubic review

@cubic-dev-ai

cubic-dev-ai Bot commented Aug 30, 2026

Copy link
Copy Markdown

@cubic review

@waleedlatif1 I have started the AI code review. It will take a few minutes to complete.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 2 files

Reply with feedback, questions, or to request a fix.

Fix all with cubic | Re-trigger cubic

Comment thread apps/sim/blocks/blocks/github.ts Outdated
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cubic review

@cubic-dev-ai

cubic-dev-ai Bot commented Aug 30, 2026

Copy link
Copy Markdown

@cubic review

@waleedlatif1 I have started the AI code review. It will take a few minutes to complete.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 2 files

Reply with feedback, questions, or to request a fix.

Fix all with cubic | Re-trigger cubic

Comment thread apps/sim/blocks/blocks/github.ts Outdated
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cubic review

@cubic-dev-ai

cubic-dev-ai Bot commented Aug 30, 2026

Copy link
Copy Markdown

@cubic review

@waleedlatif1 I have started the AI code review. It will take a few minutes to complete.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 2 files

Reply with feedback, questions, or to request a fix.

Fix all with cubic | Re-trigger cubic

Comment thread apps/sim/blocks/blocks/github.ts
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cubic review

@cubic-dev-ai

cubic-dev-ai Bot commented Aug 30, 2026

Copy link
Copy Markdown

@cubic review

@waleedlatif1 I have started the AI code review. It will take a few minutes to complete.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 2 files

Confidence score: 5/5

  • Automated review surfaced no issues in the provided summaries.
  • No files require special attention.

Re-trigger cubic

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cubic review

@cubic-dev-ai

cubic-dev-ai Bot commented Aug 30, 2026

Copy link
Copy Markdown

@cubic review

@waleedlatif1 I have started the AI code review. It will take a few minutes to complete.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 2 files

Reply with feedback, questions, or to request a fix.

Fix all with cubic | Re-trigger cubic

Comment thread apps/sim/blocks/blocks/github.param-wiring.test.ts Outdated
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cubic review

@cubic-dev-ai

cubic-dev-ai Bot commented Aug 30, 2026

Copy link
Copy Markdown

@cubic review

@waleedlatif1 I have started the AI code review. It will take a few minutes to complete.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 2 files

Confidence score: 5/5

  • Automated review surfaced no issues in the provided summaries.
  • No files require special attention.

Re-trigger cubic

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant