fix(conductor): reject a non-boolean exists in a file acceptance - #9311
Conversation
The goal-conductor acceptance evaluator read the file kind's wanted
state as bool(accept.get("exists", True)), coercing instead of
validating: any non-empty string is truthy, so an acceptance spelled
{"exists": "false"} — the shape produced whenever the acceptance was
assembled from text rather than real JSON booleans — was read as True.
An absence check silently inverted into a presence check and the
evaluator reported pass for exactly the state the spec asked to reject.
Validate before use, in the same shape as the two neighbouring guards
(the pr guard that refuses bool-as-int, and the path guard in the same
branch): a non-boolean exists now returns the error verdict
"file spec needs a boolean exists". isinstance(exists, bool) is strict
on purpose — 1/0 and "true"/"false" are all rejected, mirroring the
sibling pr field's refusal of bool as an int. The absent-key default of
True is preserved and pinned by test.
Fixes #9293
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsNo findings. False positive or not applicable? A repository writer can comment: |
Design Review (Fable 5) — ✅ PASSDesign-level review of Design-Verdict: PASS Real inversion bug, fixed at the right layer with the file's own validate-before-use idiom; strictness deliberately pinned, defaults preserved, sibling copy accounted for. [DESIGN-REVIEWED] 61283fc |
Opus 4.8 Review — ✅ no blocking findingsReviewed Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
First Principles Review (Fable 5) — ✅ PASSPremise-level review of The claims check out: only one First-Principles-Verdict: PASS A reported silent verdict inversion (#9293) is fixed at the invariant gap — the one unvalidated field in a branch whose neighbours already validate — nothing rides along. What this change shipsIntent: stop a
No undeclared items. The fix sits at cause level for this component: specs are model-authored (untrusted external content per the repo's boundary list), so validation at the read site is the invariant that was missing, not a symptom patch. I grepped [FIRST-PRINCIPLES-REVIEWED] 61283fc |
NicholasRBowers
left a comment
There was a problem hiding this comment.
Tier 1 auto-approve: fix (2 files). Criteria: no conflict, no requested changes, security path denylist clean, design-doc gate clean, SAST annotations clean, security checklist all-NO, AI reviewers green. Category: clear root cause — bool() coerced a truthy string 'false' in a file acceptance's exists field, inverting an absence check into a presence check; now rejects non-boolean exists with an explicit error, with regression tests.
Fixes #9293
Symptom
The goal-conductor acceptance evaluator decides a
fileacceptance by comparing the file's real presence against a wanted state read asbool(accept.get("exists", True)).bool()coerces instead of validating: any non-empty string is truthy, so an acceptance spelled{"exists": "false"}— the shape produced whenever the acceptance was assembled from text rather than from real JSON booleans — is read asTrue. A "this file must be gone" check silently becomes "this file must be present", and the evaluator reportspassfor exactly the state the spec asked to reject.Root cause
src/kiro_crew/builtin_skills/goal-conductor/scripts/accept_eval.py,_evaluate'sfilebranch: the one field in the branch that was coerced rather than validated. Its two neighbours already return anerrorverdict for a malformed field — theprguard (which goes out of its way to rejectboolas anintsubclass) and thepathguard in the same branch.Fix
Validate before use, in the same shape and idiom as the two neighbouring guards: a non-boolean
existsnow returns("error", "file spec needs a boolean exists"). Points that are deliberate, not incidental:Trueis preserved (read throughaccept.get("exists", True); the default is itself aboolso it passes the guard) and pinned by test.isinstance(exists, bool)is strict on purpose:1/0and"true"/"false"are all rejected. The siblingprguard already reasons in exactly this direction (it rejectsboolas anint), so acceptingintas aboolhere would contradict it. A rejected spec gets a louderrora human reads; a coerced one silently inverts a check.error("this spec is malformed"), notfail("the world is not in the wanted state") — matching both existing guards, so a conductor fixes the spec instead of re-working the item.Which copies existed at branch time
The issue names a second copy at
src/kiro_crew/builtin_skills/goal-ledger-conductor/scripts/accept_eval.pyand its byte-identical pin intest/test_ledger_conductor_agent.py. Neither exists on main at branch time — both arrive with open PR #9277 (feat/ledger-conductor-agent). This PR therefore fixes the one existing copy undergoal-conductor/; when #9277 rebases, its byte-identical pin test will correctly go red until it carries this guard into its new copy — CI-caught, not silent, and the expected cost of fixing a live defect rather than waiting on merge order.Verification
test_file_rejects_a_non_boolean_existswas written first and run against unmodified main — it fails with the exact reported inversion (assert 'pass' == 'error'for a present file with{"exists": "false"}), not an incidental error."false","true",1,0,Noneall rejected; absent key still defaults to a presence check; realTrue/Falsesemantics pinned across all four presence/wanted combinations (the regression floor).bool()coercion →test_file_rejects_a_non_boolean_existsgoes red (AssertionError: false); restore → green.isinstance(exists, bool)toisinstance(exists, (bool, int))→ the1/0cases go red (AssertionError: 1); restore → green. This pins the strictness decision rather than leaving it incidental.scripts/local-gate.py --base origin/main: backend-only diff, full backend suite run on this branch AND, with the identical bare invocation (pytest -q -n auto --dist loadgroup, no path args), on agit worktreeatorigin/main. Sorted FAILED/ERROR id sets (ANSI-stripped) diffed both directions: byte-identical except one branch-only entry intest/test_pod_seed_scenarios.py(unrelated file, passes standalone on the branch — a parallel-run flake, not a regression). This repo has a known environmental failure baseline; set identity is the proof.existsfield's type contract (greppeddocs/system-specs/modules/andgoal-conductor/SKILL.md), so no same-commit doc update is owed.No frontend change; no visual delta.
Backend-only skill-script validation fix; there is no UI surface to screenshot.Pattern harvest
Rule candidate: in a spec-parsing branch that already returns
errorverdicts for malformed fields, every field read must validate its type before use — abool()/str()/int()coercion besideisinstanceguards is the defect shape, because coercion silently inverts semantics instead of surfacing the malformed spec. Knowingly out-of-scope sibling: the secondaccept_eval.pycopy arriving with PR #9277 (will need this same guard carried into it on rebase).