Skip to content

[Bugfix #1142] Expose exitCode to cron conditions; let condition-true failure runs deliver alerts#1143

Open
waleedkadous wants to merge 7 commits into
mainfrom
builder/bugfix-1142
Open

[Bugfix #1142] Expose exitCode to cron conditions; let condition-true failure runs deliver alerts#1143
waleedkadous wants to merge 7 commits into
mainfrom
builder/bugfix-1142

Conversation

@waleedkadous

Copy link
Copy Markdown
Contributor

Summary

Fixes the two-layer bug in tower-cron.ts where (1) conditions referencing exitCode threw ReferenceError on every run, and (2) failure runs could never deliver alerts — making "alert me when this command fails" inexpressible.

Root Cause

  • evaluateCondition() built new Function('output', 'return ' + condition) — only output was in scope, so condition: "exitCode != 0" threw ReferenceError every run (12k+ WARN lines in a real tower.log). The catch forced shouldNotify = false.
  • runCommand() rejected on any non-zero exit, so the exit code was never available as data.
  • The delivery gate if (shouldNotify && result === 'success') suppressed delivery on exactly the runs a monitoring task cares about.

Fix

  • runCommand() resolves { output, exitCode }. A plain non-zero exit (numeric error.code, no kill signal) resolves with that exit code — it's data, not an error. Spawn failures and timeout/kills still reject, reported as exitCode -1 / 124 respectively.
  • evaluateCondition(condition, output, exitCode = 0) exposes both variables to the condition. Output-only conditions are unchanged.
  • Delivery gate: a task with a condition delivers exactly when the condition is truthy (including failed runs). A task without a condition keeps today's behavior — deliver only on exit 0 — so existing tasks don't start alert-spamming on flaky commands. Non-delivering failures keep the WARN log.
  • last_result in cron_tasks still records success/failure by exit code — only delivery logic changed.
  • Docs: the condition environment (output: string, exitCode: number) is now documented in the afx skill's ## afx cron section and a new ### afx cron section in agent-farm.md, in BOTH trees (instance + skeleton).

Test Plan

  • 8 new regression tests in tower-cron.test.ts (34 total, all pass):
    • exitCode != 0 condition delivers on non-zero exit, no ReferenceError
    • exitCode != 0 condition does not deliver on clean exit
    • no-condition task does not deliver on non-zero exit (regression)
    • output-only conditions unchanged alongside exitCode (regression)
    • timeout reported as exitCode 124 — condition can still fire; without a condition it stays WARN-only
    • evaluateCondition unit coverage for exitCode scope + default
  • Full suite: 3440 passed, 48 pre-existing skips. Root pnpm build passes.

Fixes #1142

🤖 Generated with Claude Code

…ion-true failure runs deliver

- runCommand resolves { output, exitCode } for plain non-zero exits instead
  of rejecting; only spawn failures and timeout/kills still reject (reported
  as exitCode -1 / 124)
- evaluateCondition takes exitCode as a second scope variable, so
  condition: "exitCode != 0" no longer throws ReferenceError
- Delivery gate: with a condition, the condition alone decides delivery
  (failure runs can now alert); without one, deliver only on exit 0 as before
- last_result semantics unchanged (success/failure by exit code)
- Regression tests for exitCode conditions, output-only conditions,
  no-condition failure runs, and the timeout path
- Document the condition environment in the afx skill and agent-farm.md,
  both trees
@waleedkadous

Copy link
Copy Markdown
Contributor Author

Architect Review

Nice, tight execution — this carries the issue's prescribed design faithfully, and the docs work goes beyond the ask in a good way.

Low-risk change (production diff is 37/22 in one file, tower-cron.ts; the rest is 204 lines of regression tests + doc sync in both trees). Verified against the issue:

  • runCommand resolves {output, exitCode} for plain non-zero exits (exit code is data), still rejects only for spawn failure and timeout/kill — with the coreutils-style 124/-1 mapping in the catch. ✓
  • evaluateCondition(condition, output, exitCode = 0) — backward-compatible default, output-only conditions untouched. ✓
  • Delivery gate: condition-bearing tasks deliver on the condition alone; no-condition tasks keep exit-0-only delivery (no alert-spam regression). ✓
  • last_result still records success/failure by exit code. ✓
  • Docs: full .af-cron YAML format + condition environment documented in the afx skill AND agent-farm.md, both trees — the skeleton's agent-farm.md previously had no cron section at all, so this fills a real gap. ✓

One intentional semantic change worth naming for the record (it's documented, and correct): an output-only condition that evaluates true on a failed run now delivers, where before failure suppressed everything. That's the point of the fix — "condition alone decides" — just noting it's a behavior change for pre-existing condition-bearing tasks, not a pure bugfix.

CMAP unanimous (3× APPROVE/HIGH), CI 6/6 green.

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.

tower-cron: conditions can't see exitCode (ReferenceError every run) and failure runs can never deliver alerts

1 participant