Skip to content

docs(workflows): clarify acceptance requiredEvidence type, limits, and usage (#207) - #223

Merged
tt-a1i merged 1 commit into
openpi-dev:mainfrom
hasak21:docs/clarify-workflow-acceptance-required-evidence
Aug 29, 2026
Merged

docs(workflows): clarify acceptance requiredEvidence type, limits, and usage (#207)#223
tt-a1i merged 1 commit into
openpi-dev:mainfrom
hasak21:docs/clarify-workflow-acceptance-required-evidence

Conversation

@hasak21

@hasak21 hasak21 commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Problem

Workflow API callers can easily mistake acceptance.criteria[].requiredEvidence for a prose description field instead of a concise string array (string[]). Passing a scalar string currently fails invocation admission with an error without clear documentation guidance and lacks direct regression test coverage in acceptance unit tests (Fixes #207).

Value

Clarifying the parameter type, character/length limits (1–32 criteria, ≤16 evidence labels, ≤120 chars per label), and exact match requirements prevents caller confusion and provides explicit contract documentation and regression protection.

Approach

  1. Update skills/workflows/REFERENCE.md to define requiredEvidence?: string[], specify the 1–32 criteria and 16-label limits, and clarify exact matching against returned acceptance.criteria[].evidence.
  2. Update README.md with an explicit acceptance example and field/constraint explanations.
  3. Add regression tests in tests/extensions/workflows/acceptance.test.ts verifying that passing scalar strings, >16 labels, blank strings, or oversized labels fail with appropriate errors.

Validation

  • bun run check - passed (format, lint, typecheck)
  • bun run test - 965 tests passed (935 Node + 30 Vitest), 0 failures
  • git diff --check - passed

Impact

  • User-visible behavior: none (runtime rejection behavior remains unchanged).
  • Model-visible context/tools: Workflow skill reference and README contain explicit requiredEvidence typing and examples.
  • Runtime/lifecycle: none.
  • Persisted config/data: none.
  • Compatibility or risk: none; existing acceptance validation semantics and limits are preserved.

@agnitum2009 agnitum2009 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

APPROVE — independent verification against cc1b174; all claims confirmed.

Local verification (fetched the branch and ran the stated test plan):

  • bun run check -> exit 0
  • bun run test -> 965 pass (935 Node + 30 Vitest), matching the PR description exactly
  • New acceptance tests: 6/6 in tests/extensions/workflows/acceptance.test.ts

Docs <-> implementation cross-check (extensions/workflows/acceptance.ts): every documented constraint matches the code — criteria 1-32 (MAX_CRITERIA=32, zero rejected), description 1-500 chars, requiredEvidence must be a string[], up to 16 labels, each label non-blank and up to 120 chars ("invalid evidence label"), exact-match evidence via result.evidence.includes(required). The new tests assert the actual error messages, so they are genuine regression protection.

Diff is docs + tests only — no runtime behavior change, consistent with the stated impact.

Non-blocking nits: the blank-label rule could be named in README, and description length is measured after trim — both cosmetic.

@tt-a1i

tt-a1i commented Aug 29, 2026

Copy link
Copy Markdown
Collaborator

This focused Workflow documentation and regression-test PR remains open under #277. Its next gate is an update to current main, exact-head review of the requiredEvidence behavior contract, and fresh CI. No merge action is taken by this triage.

@hasak21
hasak21 force-pushed the docs/clarify-workflow-acceptance-required-evidence branch from cc1b174 to dcb1b3e Compare August 29, 2026 06:11

@tt-a1i tt-a1i left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Independent exact-head review: solves the requiredEvidence string-array documentation gap without changing runtime semantics; documents current criteria/label limits and exact matching; adds focused rejection coverage. Focused acceptance tests 6/6, bun run check, diff-check, and all three GitHub CI jobs passed. No findings.

@tt-a1i
tt-a1i merged commit a15a0d8 into openpi-dev:main Aug 29, 2026
3 checks passed
agnitum2009 pushed a commit to agnitum2009/openpi that referenced this pull request Aug 29, 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.

Clarify acceptance.requiredEvidence type and usage in Workflow API

3 participants