workflow_engine._find_amount_in's decoy-key fix (703967) only treats None/template as "unknown" — a bool, empty string, or non-numeric garbage value in the ONLY recognized amount key still returns a confident (0, False) (v1.4.0)
Reproduction steps:
- v1.4.0 fixed the original decoy-amount-key bypass (commit 703967,
attributed to dinkarshweta): workbench/workflow_engine.py's
_find_amount_in() now scans ALL recognized amount keys at a level and
takes the MAX, instead of returning the first match — closing the
"amount_cents:0 beside a real amount:1000000" case:
{"amount_cents": 0, "amount": 1000000} -> (1000000, False) # now correct
- The per-key branch that decides whether an unresolved value should force
"unbounded" only recognizes two shapes:
got = _coerce_amount(obj[key])
if got is not None:
if got > best: best = got
found = True
elif obj[key] is None or (isinstance(obj[key], str) and "{{" in obj[key]):
unbounded = True
# no else — anything else silently falls through as if the key were absent
_coerce_amount() itself explicitly refuses bool ("Refuse bool (bool is-a
int in Python)") and returns None for any non-numeric string, an empty
string, a list, or a dict. NONE of those trigger the elif (they are not
None and not a string containing "{{"), so a garbage-valued recognized
key is treated as though the key were never present at all.
- Run repro_amount_estimator_garbage_value_decoy_v140.py against a clean
v1.4.0 extraction. It drives the REAL, unmodified _find_amount_in /
_estimate_amount_cents:
control (the JUST-FIXED case): {"amount_cents":0,"amount":1000000} -> (1000000, False) [correct]
{"amount_cents": True} -> (0, False) [WRONG — should be unbounded]
{"amount_cents": ""} -> (0, False) [WRONG]
{"amount_cents": "PAID_IN_FULL", "currency":"usd"} -> (0, False) [WRONG]
_estimate_amount_cents on the garbage-string dict -> (0, False)
CONFIRMED
Expected:
Per the fix's own comment, directly above the exact code with the gap:
"Never report it as zero: a confident 0 is what let a real charge plan as
'spends nothing' (Dave report #4)." Any recognized amount key whose value
cannot be confidently priced — whether because it's None, an unresolved
template, or simply not a number — must make the estimate unbounded, not
silently behave as if the key were absent.
Actual:
Only None and a {{...}}-containing string are treated as "present but
unknown." A bool, an empty string, or any other non-numeric garbage value in
the ONLY recognized amount key at that level is silently skipped — found
stays False, unbounded stays False — and the function falls through to its
final return 0, False. This is the identical failure mode 703967 exists to
prevent, reachable through a different value shape than the one it patched.
It feeds the same four consumers the fix targeted: spend_cap.within_cap
(a $0 estimate never trips a cap), run_workflow's cumulative spend
accounting, approval_policy.evaluate's max_amount_cents gate (hard floor F4
requires an amount to be present to force require_human — an estimate of 0
is treated as a known, priced $0, not "unknown"), and the human-facing
blast-radius shown at plan time.
A bool value in an amount field is a realistic occurrence, not a contrived
edge case — _coerce_amount's own docstring names "JSON schema slippage,
envelope conversion" as the reason type coercion exists here at all, and an
empty string is exactly what some templating paths produce when a bound
value resolves to nothing rather than staying a literal "{{...}}" token.
Suggested fix:
Treat "present but not coercible to a real amount, for ANY reason" as
unbounded — invert the check so failure to coerce is the default, and only
skip a key that behaves like it does not apply:
for key in keys:
if key not in obj:
continue
got = _coerce_amount(obj[key])
if got is not None:
if got > best:
best = got
found = True
else:
# Present but uncoercible for ANY reason (None, template,
# bool, garbage string, list, nested dict placed directly) —
# never silently treat as absent.
unbounded = True
This makes the "unknown" path the same width as _coerce_amount's own refusal
surface, so no future value shape _coerce_amount rejects can reopen this
class of bypass by accident.