fix(github): wire the block fields whose ids do not match their tool params - #7287
fix(github): wire the block fields whose ids do not match their tool params#7287waleedlatif1 wants to merge 7 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Greptile SummaryThe 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.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| 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
Reviews (7): Last reviewed commit: "test(github): exercise the isSet guard i..." | Re-trigger Greptile
|
@cubic review |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
All reported issues were addressed across 2 files
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
|
@cubic review |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
All reported issues were addressed across 2 files
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
|
@cubic review |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
All reported issues were addressed across 2 files
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
|
@cubic review |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
|
@cubic review |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
All reported issues were addressed across 2 files
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
|
@cubic review |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
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), andGitHubBlock.tools.configdeclared onlytool:, with noparamsmapper and nocanonicalParamIdanywhere in the file. So a subBlock namedreaction_contentcan never populate a tool param namedcontent.Broken outright — the call dies before the HTTP request
validateRequiredParametersAfterMerge(tools/utils.ts:111-138) throws for any missing requireduser-or-llmparam, so these three fail with an error naming a field that does not exist in the UI:contentreaction_contentContent is required for GitHub Create Issue Reactioncontentreaction_contentContent is required for GitHub Create Comment Reactiontitlemilestone_titleTitle is required for GitHub Create MilestoneSilently inert — no error, the input is just dropped
gist_public→public(every gist created secret regardless of the selection),fork_name→name,fork_sort→sort,milestone_title/milestone_description→title/descriptionon update,milestone_state→state,milestone_sort→sort.The fix
One
tools.config.paramsmapper, driven by an alias table where each entry names the operations whose tool actually declares its target.GitHubV2Blockalready forwardsconfig.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-191merges{ ...inputs, ...params(inputs) }, andproviders/utils.tsinstalls this same function as the providerparamsTransform, spreading its result over the model's tool-call arguments. An unconditional write would clobber a model-suppliedcontent/title/sortwithundefined— 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-emptymode: 'advanced'field without evaluating its condition, and seven of the eight sources are advanced. So amilestone_titleleft behind from Create Milestone is still inparamsafter the user switches to Update PR — an unscoped alias would rewrite it totitleand overwrite the PR title the user typed. Scoping also subsumes the collision problem:sorthas two sources, andtitle/description/stateshare names with other operations' fields.gist_publicalso needs coercion, not just a rename: it is a dropdown, so it stores the string'true'/'false'while the tool declarespublicas a boolean, and the generic handler only JSON-parsesjson/array-typed inputs (generic-handler.ts:193-200). This follows the established pattern inblocks/blocks/ashby.ts:1083andblocks/blocks/agentmail.ts:590. Presence is tested rather than truthiness so a deliberatefalseis not mistaken for an unset field — the block declares this input asboolean, so a writer following that schema storesfalserather than the dropdown's'false', and a truthy check would silently drop one of them.Why not
canonicalParamIdIt 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 becomesbasicId, and the other's value becomes invisible to the collapse atserializer/index.ts:589-610.fork_sortandmilestone_sortboth targetsort, 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_protection—restrictionshas no subBlock at all, three required params aremode: 'advanced'and unmarked, and two need JSON/boolean coercion. That operation needs redesigning, not rewiring.pagesubBlock, conditioned ongithub_list_tags. Twenty-two other list tools declarepageand can never receive it.sortcovers 1 of 5 search ops,per_pagemisses 3 ops,labels/assigneesmissupdate_issue,draft/prereleasemissupdate_release.github_check_starcannot returnstarred: false— GitHub signals not-starred with 404 and the shared executor throws attools/index.ts:2838beforetransformResponse. Fixing it means either an internal-operation conversion or a shared-executor change affecting ~10 other tools' dead branches.workflow_idrenders forgithub_list_workflow_runs, which declares no such param.operationnever reaches a block'sparamsmapper on the agent tool-calling path.providers/utils.tsspreads it in for tool selection (:736-739) but builds the transform's input fromblock.paramsalone (: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 onparams.operationare 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
requiredflips; zerovisibilitychanges — the diff adds a mapper and a test file and touches nothing else.check-block-registry.ts origin/stagingpasses 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:
<Field> is requiredtoday, so nothing can depend on their current behavior. Strictly better, zero risk.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:
'false''true'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 (
isNonEmptyValueis false for nullish/empty), so the default path is untouched.A stale list filter also cannot leak into a write:
milestone_stateis scoped togithub_list_milestonesonly, 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 acrossblocks/blocks/githubandtools/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/undefinedtreated as not-provided;gist_publiccoercion 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_titlemust not becomegithub_update_pr'stitleand must not clobber a title the user typed, swept across all seven advanced sources against unrelated operations; and an absent, undefined, empty or non-stringoperationemits 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_publiccheck turns 2 red.bun run lint,bun run type-check(no GitHub diagnostics),bun run check:audits(39 audits), andcheck-block-registry.ts origin/stagingall pass.tool-metadata:generateandgenerate-docsproduce no drift.Type of Change
Checklist