feat: report error when @utils.setVariables assigns to undefined variable#59
feat: report error when @utils.setVariables assigns to undefined variable#59awli wants to merge 6 commits into
Conversation
Add set-variables-io lint pass that validates `with` clause parameters in @utils.setVariables reasoning actions against declared variables. Produces a 'set-variables-unknown-variable' error with typo suggestions. Resolves W-22695026
…s-io Use AstNodeLike narrowing interfaces (ReasoningActionBlock, WithClauseNode) and isAstNodeLike() guards instead of Record<string, unknown> casts.
Replace __kind string checks + cast patterns with proper type guard functions (isReasoningActionBlock, isWithClause) that narrow the type directly.
AstRoot already extends AstNodeLike, so no cast is needed to access index-signature properties on root.
monga-puneet
left a comment
monga-puneet
left a comment
There was a problem hiding this comment.
Two findings — both correctness/maintainability. Details inline.
| .filter(raBlock => isSetVariablesAction(raBlock.value)) | ||
| .map(raBlock => raBlock.statements) | ||
| .filter(statements => statements !== undefined)) { | ||
| for (const stmt of statements.filter(isWithClause)) { |
There was a problem hiding this comment.
Mutability not enforced. The check only verifies the variable exists, not that it's mutable. @utils.setVariables should not be able to assign to a linked (context-bound) variable, but the test at lint.test.ts:4845 explicitly asserts no error for that case.
Example that passes lint today but is likely a real bug:
variables:
context_val: linked string
subagent main:
reasoning:
actions:
update: @utils.setVariables
with context_val="test" # silently allowed
Suggest: typeMap.variables.get(stmt.param)?.mutability !== 'mutable' → emit a distinct diagnostic (e.g. set-variables-immutable-target).
| ...resolveNamespaceKeys('topic', ctx), | ||
| ]); | ||
|
|
||
| for (const topicMap of [...subagentKeys] |
There was a problem hiding this comment.
Reinvents the walker. The sibling rule action-io.ts:28-36 does the same kind of validation by consuming reasoningActionsKey via defineRule({ deps: { entry: each(reasoningActionsKey) } }) — it gets the traversal, statement extraction, and typed WithClause/SetClause shapes for free. This pass instead manually walks subagent/topic namespaces and reaches into block.reasoning?.actions with as any + an eslint-disable.
Consequence: if reasoning AST shape changes, action-io keeps working but this pass silently breaks. Refactor to filter reasoningActionsKey entries where the action ref is @utils.setVariables, then iterate entry.statements — drops ~60 lines and the as any.
monga-puneet
left a comment
monga-puneet
left a comment
There was a problem hiding this comment.
Left couple of comments
setu4993
left a comment
setu4993
left a comment
There was a problem hiding this comment.
Looks good, 1 question about why we're not parsing through procedurals.
| it('does not report error for linked variables', () => { | ||
| const diagnostics = runLint(` | ||
| variables: | ||
| context_val: linked string | ||
| name: mutable string | ||
| subagent main: | ||
| label: "Main" | ||
| reasoning: | ||
| instructions: -> | ||
| |Do it | ||
| actions: | ||
| update: @utils.setVariables | ||
| description: "Update" | ||
| with name=... | ||
| with context_val="test" | ||
| `); |
There was a problem hiding this comment.
This should be an error, right? We shouldn't be able to assign?
There was a problem hiding this comment.
I guess it'd be fine if this check doesn't flag here but a different one would disallow assignment to linked vars generally.
There was a problem hiding this comment.
Oh, that's a good point. I guess we're not allowed to change linked vars. But we should just cover both cases now.
| for (const reasoningActions of [...topicMap.values()] | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| .map(block => (block as any).reasoning?.actions) | ||
| .filter(isNamedMap)) { |
There was a problem hiding this comment.
We should also look for these in procedurals, no?
3 participants
What
Why
How
Test Plan
pnpm test)pnpm lint && pnpm typecheck)Checklist