Skip to content

fix(conductor): reject a non-boolean exists in a file acceptance - #9311

Merged
NicholasRBowers merged 1 commit into
mainfrom
fix/accept-eval-exists-not-bool-9293
Sep 7, 2026
Merged

fix(conductor): reject a non-boolean exists in a file acceptance#9311
NicholasRBowers merged 1 commit into
mainfrom
fix/accept-eval-exists-not-bool-9293

Conversation

@dwu96

@dwu96 dwu96 commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Fixes #9293

Symptom

The goal-conductor acceptance evaluator decides a file acceptance by comparing the file's real presence against a wanted state read as bool(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 as True. A "this file must be gone" check silently becomes "this file must be present", and the evaluator reports pass for exactly the state the spec asked to reject.

Root cause

src/kiro_crew/builtin_skills/goal-conductor/scripts/accept_eval.py, _evaluate's file branch: the one field in the branch that was coerced rather than validated. Its two neighbours already return an error verdict for a malformed field — the pr guard (which goes out of its way to reject bool as an int subclass) and the path guard in the same branch.

Fix

Validate before use, in the same shape and idiom as the two neighbouring guards: a non-boolean exists now returns ("error", "file spec needs a boolean exists"). Points that are deliberate, not incidental:

  • The absent-key default of True is preserved (read through accept.get("exists", True); the default is itself a bool so it passes the guard) and pinned by test.
  • isinstance(exists, bool) is strict on purpose: 1/0 and "true"/"false" are all rejected. The sibling pr guard already reasons in exactly this direction (it rejects bool as an int), so accepting int as a bool here would contradict it. A rejected spec gets a loud error a human reads; a coerced one silently inverts a check.
  • The verdict is error ("this spec is malformed"), not fail ("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.py and its byte-identical pin in test/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 under goal-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

  • Red before green: test_file_rejects_a_non_boolean_exists was 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.
  • Coverage: "false", "true", 1, 0, None all rejected; absent key still defaults to a presence check; real True/False semantics pinned across all four presence/wanted combinations (the regression floor).
  • Mutation checks, both directions recorded:
    • Reverting the guard to the original bool() coercion → test_file_rejects_a_non_boolean_exists goes red (AssertionError: false); restore → green.
    • Widening isinstance(exists, bool) to isinstance(exists, (bool, int)) → the 1/0 cases 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 a git worktree at origin/main. Sorted FAILED/ERROR id sets (ANSI-stripped) diffed both directions: byte-identical except one branch-only entry in test/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.
  • Frontend cross-surface guard specs (the gate's 219-file vitest selection) run green.
  • Docs: no spec module or SKILL.md documents the exists field's type contract (grepped docs/system-specs/modules/ and goal-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 error verdicts for malformed fields, every field read must validate its type before use — a bool()/str()/int() coercion beside isinstance guards is the defect shape, because coercion silently inverts semantics instead of surfacing the malformed spec. Knowingly out-of-scope sibling: the second accept_eval.py copy arriving with PR #9277 (will need this same guard carried into it on rebase).

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
@dwu96
dwu96 requested a review from a team as a code owner September 7, 2026 21:34
@dwu96
dwu96 requested a review from CrysisDeu September 7, 2026 21:34
@github-actions github-actions Bot added the readiness: checking Automated validation is still running label Sep 7, 2026
@github-actions

github-actions Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 61283fc6842fa5546c1a05ed1fde4400b7b90265 and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 61283fc

False positive or not applicable? A repository writer can comment:
/ai-review override gpt 61283fc6842fa5546c1a05ed1fde4400b7b90265: <one-sentence reason>

@github-actions

github-actions Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — ✅ PASS

Design-level review of 61283fc6842fa5546c1a05ed1fde4400b7b90265 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

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

@github-actions

github-actions Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

Reviewed 61283fc6842fa5546c1a05ed1fde4400b7b90265 — this comment is updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] 61283fc

Verdict parsed from the review's SHA-scoped output markers for commit 61283fc6842fa5546c1a05ed1fde4400b7b90265.

False positive or not applicable? A repository writer can comment:
/ai-review override fable 61283fc6842fa5546c1a05ed1fde4400b7b90265: <one-sentence reason>

@github-actions

github-actions Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5) — ✅ PASS

Premise-level review of 61283fc6842fa5546c1a05ed1fde4400b7b90265 — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

The claims check out: only one accept_eval.py copy exists on this branch (globbed builtin_skills/*/scripts/accept_eval.py — 1 match), and I verified the sibling-coercion question: within _evaluate, the only remaining coercion of a spec field is str(repo) at line 153, which fails loudly at gh rather than silently inverting a verdict, so it doesn't share the fixed defect's harm. The diff is exactly the guard plus its tests — no riders.

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 ships

Intent: stop a file acceptance spelled {"exists": "false"} from passing as a presence check — a FIX.

  1. A non-boolean exists now yields a loud error verdict instead of a coerced pass/fail — justified (reported defect accept_eval.py: reject a non-boolean exists instead of bool()-coercing it #9293).
  2. New error text "file spec needs a boolean exists" surfaces to conductors — justified, it is the fix's observable form and matches the two neighbouring guards' idiom.
  3. Absent-key default (exists: true) unchanged and now pinned — justified regression floor, declared.

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 _evaluate for other spec-field coercions sharing the silent-inversion shape: str(repo) (accept_eval.py:153) is the only coercion left, and a garbage repo fails loudly at gh — 0 unfixed siblings of the actual defect. The description's claimed second copy under goal-ledger-conductor/ genuinely does not exist on this branch (1 glob match), so the cross-PR handoff account is accurate, not an unfixed sibling here.

[FIRST-PRINCIPLES-REVIEWED] 61283fc

@github-actions github-actions Bot added readiness: passed Eligible automated validation passed for the current revision and removed readiness: checking Automated validation is still running labels Sep 7, 2026
@NicholasRBowers
NicholasRBowers enabled auto-merge (squash) September 7, 2026 22:24

@NicholasRBowers NicholasRBowers left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@NicholasRBowers
NicholasRBowers merged commit 678fc32 into main Sep 7, 2026
63 checks passed
@NicholasRBowers
NicholasRBowers deleted the fix/accept-eval-exists-not-bool-9293 branch September 7, 2026 22:25
@github-actions github-actions Bot removed the readiness: passed Eligible automated validation passed for the current revision label Sep 7, 2026
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.

accept_eval.py: reject a non-boolean exists instead of bool()-coercing it

2 participants