Skip to content

Correctness sub-agent: detect GitHub Actions expression evaluation pitfalls #143

Description

@fullsend-ai-retro

What happened

On PR #1611, human reviewer waynesun09 caught a HIGH bug: the ternary expression condition && '' || fallback in dispatch.yml always evaluates to fallback because empty string '' is falsy in GitHub Actions expressions. This meant the role-based kill switch was completely bypassed — when role-check set skipped=true, the expression still returned the stage name and the workflow ran. The review bot missed this across all 4 review cycles. waynesun09 also noted that fromJSON() in concurrency groups could hard-fail at workflow parse time on malformed payloads — another GHA expression pitfall the bot did not flag.

What could go better

The correctness sub-agent (skills/pr-review/sub-agents/correctness.md) has no specific guidance about GitHub Actions expression semantics. GHA expressions have non-obvious truthy/falsy rules that differ from most programming languages: empty string is falsy, 0 is falsy, and fromJSON() evaluates at parse time. The condition && '' || fallback anti-pattern is especially dangerous because it looks correct to anyone familiar with JavaScript or shell ternary idioms. No existing open issue covers GHA expression evaluation pitfalls specifically — #1275 is about tracing workflow_call chains and #1895 is about backward-incompatible input schema changes. High confidence this is actionable: the pattern is well-defined and easy to detect with specific guidance.

Proposed change

Add a 'GitHub Actions expression pitfalls' section to skills/pr-review/sub-agents/correctness.md covering: (1) Empty string '' is falsy — condition && '' || alternative always yields alternative, making it unsuitable for ternary-style expressions. The correct pattern is condition && 'empty-sentinel' || alternative or a multi-step approach. (2) fromJSON() evaluates at workflow/job parse time, not at step runtime — malformed input causes opaque parse errors, not catchable step failures. (3) 0, false, null, and '' are all falsy; only non-empty strings and non-zero numbers are truthy. Include before/after examples for each pattern.

Validation criteria

Run the correctness sub-agent against a test diff containing a workflow with the condition && '' || fallback pattern. The sub-agent should flag it as a logic bug, explaining that empty string is falsy in GHA expressions and the expression always evaluates to the fallback value.


Generated by retro agent from fullsend-ai/fullsend#1611

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions