feat(space): acceptance.v1 contract for human acceptance verdicts

[PeterGuy326]

Summary

Adds the unreleased acceptance.v1 contract for a human verdict on completed work in a collaboration space. Approval remains permission to act; this contract describes acceptance of a deliverable. This is a shared contract plus tests, with no persisted store or renderer.

An accepted record must contain one passed verdict for each index 0..criteriaCount-1. criteriaCount is required on records and create requests, bounded to 64, and must come from the plan being accepted. Consumers must compare it with the authoritative document revision: validation checks the record shape, not caller identity or authority. Rejected records require a nonblank note.

Review corrections

• Require complete coverage against an explicit criteria count; sparse, partial and out-of-range verdict sets fail closed.
• Enforce the exported 128-character bound on every identifier.
• Require a real UTC ISO timestamp, rejecting shorthand dates and calendar rollovers.
• Reject unsafe controls/bidi characters and absolute or traversal artifact pointers. File consumers must still enforce filesystem confinement and symlink policy.
• Bound notes even when blank; cover all boundaries with regression tests.
• Runtime inventory and ./acceptance export remain registered; CHANGELOG has an Unreleased entry.

Validation ledger

Environment: macOS arm64, Node 24.14.1, npm 11.11.0. No external service or credential is required. From the repository root, install dependencies with npm ci; the complete UI check also needs the CI-pinned design-system checkout at 4a82fce41f89f1a55b6b02cdb54f7a4e9ce9d84d, built in the adjacent design-system folder.

ID	Acceptance criterion	Command	Expected	Observed
V1	Contract compiles under repository flags	npm run build	Exit 0	PASS, exit 0
V2	Complete coverage, bounded ids/notes/paths, real timestamps and rejection notes	node --test apps/server/dist/test/acceptance.test.js	No failures	PASS, 24/24
V3	New shared module is packaged and imports close over inventory	npm run prepare:updater && node --test scripts/test/package-layout.test.mjs	No failures	PASS, 9/9
V4	Full platform verification and security	GitHub verify / Security Baseline on updated head	All checks green	FAIL: both Node 24 jobs reach npm audit --audit-level=high and fail on existing http-cache-semantics@4.2.0 (GHSA-ch52-4w7c-c8xp). The other 9 checks, including CodeQL, pass

Compatibility, limitations and rollback

criteriaCount is a required field added before v1 ships; the stacked UI PR #560 has been updated with it. No released persistence is migrated. This contract does not enforce the GoalWorkItem state transition or authenticate an acceptor. Those belong to future consumers. Real Windows execution was not reproduced locally; hosted CI covers it. Reverting the additive contract and its inventory/export/test entries rolls back this slice.

Refs #558. Merge this contract before the UI; #560 is stacked onto this branch and remains a draft until document/persisted-verdict integration is complete.

Current shared audit blocker: GHSA-ch52-4w7c-c8xp. A separate 4.3.0 experiment clears npm's version-range check but still reproduces the shared-cache reuse issue, so it was not published as a security repair. No audit threshold or required check was weakened.

2026-10-05 follow-up

The current head additionally contains the dependency removal tracked in #562: app-builder-lib now uses the official @electron/get 5.1.0 already used by Electron, removing got/cacheable-request/http-cache-semantics instead of merely escaping the advisory range. Minimum build Node is 22.12.0; CI still runs Node 24 and the unchanged high-severity audit. Legacy Got download options and GLOBAL_AGENT_* proxy options are not supported by get 5; repository release commands use the existing native Electron distribution.

Fresh local validation after this dependency change: npm ci --ignore-scripts --no-audit --no-fund (exit 0), node --test scripts/test/electron-download.test.mjs (2/2, real loopback artifact download, checksum verification, validated cache reuse, corrupt SHA256 rejection), npm run security:check (exit 0; no high/critical findings, two existing moderate Vitest findings). The earlier feature-specific results above describe the feature implementation; earlier red audit CI refers to the previous head. Fresh complete current-head CI is running and has not yet been declared passed. Another maintainer must review the current head; no self-approval or branch-protection bypass.

Connection-draft prerequisite and failed-CI follow-up

The prior macOS run at 6b7819c failed four renderer tests: a real missing-PAT assertion in the old ServiceConnections implementation, two App navigation timeouts (5000ms) and a subsequent missing-tree assertion. The acceptance contract's own 24 tests passed. Rather than retrying to hide PAT loss, this head merges the already verified #513 fix. Merge #513 before this contract PR to keep the baseline dependency clear. The contract API itself is unchanged by this follow-up.

Fresh local renderer typecheck passed; App plus service-connections regression suites passed 118/118, including the workspace/model race cases that timed out in that macOS run. No assertion, exclusion or test timeout was weakened. Fresh full CI is running for the updated head.

Current-head CI result

All 11 checks passed at 0a5479c4078e4cdc50dfc46d141638df171b71b8, including both full Node 24 jobs, required security checks, staging, installers and layout parity: verify. The failure at the preceding head is retained above; it is not the current CI result. Independent current-head maintainer approval remains required.

[PeterGuy326]

Review: acceptance.v1 contract (head 1a19cd5)

Substantive review, COMMENT only (no approve/merge — author is the repo owner). CI is green. I reproduced every claim below by executing the real acceptance.ts (Node 22 --experimental-strip-types), running the 17 cases, and running a coarse mutation pass. Findings are ordered by severity; none block packaging, but the first two are domain-correctness issues worth fixing before the follow-up UI lands on top.

Constraint checks

1. runtime-layout.cjs whitelist — PASS (verified, not eyeballed). scripts/test/package-layout.test.mjs asserts SHARED_RUNTIME_FILES.filter(startsWith("dist/")) deep-equals the sorted walkFiles(packages/shared/src)/*.ts. I replayed that assertion with the PR applied: the 38 dist/ entries match the 38 source modules, and dist/acceptance.js is inserted at index 0 of the dist/ block (before dist/agent-binding.js), which is the correct alphabetical slot. git ls-tree on the head sha confirms 38 .ts files. The package.json ./acceptance subpath is inserted after ./goals; that file is not order-tested and is not alphabetical by house style, so goals -> acceptance -> task-board is fine.
2. #146 CJK gate — no violation, one note. The gate (apps/desktop/renderer/test/i18n-cjk-gate.test.ts) scans only renderer/src and packages/ui/src, so packages/shared/src/acceptance.ts is out of scope — and it contains zero CJK codepoints anyway (verified). The test file, however, uses CJK string literals as fixtures ("产出/增量更新.md" in apps/server/test/acceptance.test.ts:36,53,152,157). Not a gate failure, but it is the only place in the new surface carrying CJK literals; consider ASCII fixture paths ("artifacts/delta-update.md") to avoid seeding a pattern.
3. fail-closed — mostly PASS. Allowlisted keys at every level (top, source, each verdict), closed enum for decision, bounded text/lengths, Object.hasOwn for required keys. Unknown nested keys are rejected (source.sneaky, a verdict extra key both return acceptance_unknown_field) — good. Gaps are listed as issues below.
4. Error-code semantics — PASS on the headline cases. acceptance_incomplete_verdicts (accepted + empty/any-false verdicts), acceptance_missing_note (rejected + absent/blank/whitespace note), acceptance_duplicate_reference (repeat criteriaIndex), and acceptance_invalid_enum (decision off-allowlist) each fire on exactly one condition; the earlier-round conflict did not survive. Remaining nits below.
5. TypeScript narrowing — PASS. The module uses named interfaces (AcceptanceVerdict, AcceptanceSource, AcceptanceValidationFailure, AcceptanceValidationSuccess<T>) and a named AcceptanceValidationResult<T> union, not anonymous object-literal unions. It compiles clean under the repo's exact flags (--strict --noUncheckedIndexedAccess --target es2022 --module nodenext) with zero errors.

Issues

1. Must-fix — "accepted requires every criterion" is not actually enforced.
checkDecisionConsistency (packages/shared/src/acceptance.ts:236-243) only checks verdicts.length !== 0 && every(passed). There is no criteria list or criteria count on AcceptanceRecord/AcceptanceCreateRequest, so the stated invariant ("An accepted record must cover every criterion", line 31, and issue #558's acceptance box) is unverifiable. Reproduced:

accepted + verdicts [{criteriaIndex:5, passed:true}]        -> OK   (gaps 0-4 unrecorded)
accepted + verdicts [{0,true},{99,true}]                    -> OK   (99 not contiguous)
accepted + verdicts [{0,true},{2,true},{4,true}]            -> OK   (skips 1,3)

The sibling goals.ts carries acceptanceCriteria: string[] and validates it (validateCriteria, goals.ts:274); acceptance dropped the field, so a human "accept" can record a single passed criterion while criteria 0–4 were never judged. Since this PR exists to close the "human is the blocker" gate, an accepted record that silently under-covers is the one failure mode the contract was supposed to prevent.
Suggestion: either add a bounded criteriaCount: number (or a criteria id list) and assert verdicts.length === criteriaCount plus new Set(verdicts.map(v => v.criteriaIndex)).size === criteriaCount with indices proven to be 0..criteriaCount-1, or, if the criteria set is intentionally external, require criteriaIndex values to be exactly 0..n-1 in a contiguous set and document that the count comes from the plan side.

2. Should-fix — ACCEPTANCE_MAX_ID_LENGTH = 128 is exported but dead; identifiers actually allow 256.
packages/shared/src/acceptance.ts:22 declares ACCEPTANCE_MAX_ID_LENGTH = 128, but identifier() (line 128) always calls nonEmptyText(value, field, ACCEPTANCE_MAX_SHORT_TEXT_LENGTH) (256). The constant is referenced nowhere in the module (grep confirms a single occurrence, the declaration). Reproduced:

spaceId 200 chars -> OK    (over the documented 128 bound)
spaceId 300 chars -> acceptance_text_too_long

Public consumers reading the exported 128 constant will build a 129-char id and be surprised it validates. Suggestion: pass ACCEPTANCE_MAX_ID_LENGTH for acceptanceId/spaceId/decidedBy/positionId/turnId, or delete the constant. Add a boundary test at 128/129.

3. Should-fix — decidedAt accepts non-ISO-8601 strings via Date.parse.
iso8601 (acceptance.ts:151-160) only rejects when Date.parse is NaN, which is very permissive:

"2026-10-01"            -> OK   (date-only, not a timestamp)
"2026"                  -> OK
"1"                     -> OK   (parsed as year 2001)
"2026-02-30T00:00:00Z"  -> OK   (Feb 30 does not exist)
"Fri Oct 02 2026"       -> OK   (local-format)

goals.ts already has a stricter calendarDate helper (line 287-295) that round-trips through toISOString. For an audit trail of "who accepted, when", the record should pin a real instant. Suggestion: require an ISO-8601 instant (/^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}(\.\d+)?Z$/) and round-trip check, mirroring goals.ts.

4. Minor — acceptance_invalid is overloaded across many distinct triggers.
The code covers: non-object input, non-array verdicts, verdict count over ACCEPTANCE_MAX_VERDICTS, non-integer/negative criteriaIndex, non-boolean passed, non-string note, control characters, whitespace in an identifier, and bad decidedAt. The sibling goals.ts splits these out (goal_too_many_items, goal_invalid_transition). Callers cannot distinguish "too many verdicts" from "criterion index must be non-negative" without string-matching the message. Suggestion: add a dedicated acceptance_too_many_verdicts (matching the goals.ts convention) at minimum, and consider splitting "must be a string/boolean" type errors from value errors.

5. Minor — acceptance_unknown_field also fires on missing fields and non-objects.
keysMatch combines "no unknown key" and "all required keys present", so a missing spaceId and a non-object null/[] both return acceptance_unknown_field. This matches an existing house convention in goals.ts (verified: missing title -> goal_unknown_field), so it is not new — but since the prompt flags this class, worth noting for a v2: a missing-field case would read better as acceptance_invalid or a dedicated acceptance_missing_field.

6. Minor — control-character check misses DEL and C1.
nonEmptyText (line 121) and parseNote (line 266) test charCodeAt(0) < 0x20, so U+007F (DEL), U+0085 (NEL), and U+009F pass through into ids/paths/notes. U+202E (RTL override) also passes and could spoof a display of spaceId. Verified: spaceId "a\u007fb" -> OK, "a\u0085b" -> OK, "a\u009fb" -> OK. Suggestion: extend the predicate to < 0x20 || (>= 0x7f && <= 0x9f) and consider rejecting bidi controls (\u200e, \u200f, \u202a-\u202e, \u2066-\u2069).

7. Minor — evidencePath/artifactPath are unvalidated strings.
pathText (line 136) only reuses nonEmptyText with the 1024 bound; no separator/traversal/absolute-path policy. evidencePath: "../../etc/passwd" validates OK. If a consumer later resolves these against the space folder, that is a traversal primitive. The contract comment says "pointer to the artifact"; pin it down (relative, no .., no leading /) or explicitly document that the field is opaque and must be re-validated by the consumer.

Test coverage

17 cases, all pass (I ran them against the real module). Mutations confirmed the suite genuinely pins: duplicate detection, the all-passed requirement, the missing-note requirement, whitespace-in-identifier, the schemaVersion gate, the integer check, and the unknown-field/missing-required behaviour.

Survivors — behaviors with no asserting case (mutating the code kept all 17 green):

• the ACCEPTANCE_MAX_VERDICTS (64) bound — never exercised (issue #4 ties in here);
• the control-character rejection in both nonEmptyText and parseNote (issue #6);
• the iso8601 Date.parse rejection path (issue #3);
• pathText's 1024 bound and ACCEPTANCE_MAX_TEXT_LENGTH on note.

Two cases in the title list are thin: "whitespace id" only covers the interior-space path, not a whitespace-only id ("" -> acceptance_empty_string) or a Unicode-space id (U+00A0/U+3000 reject correctly, U+200B currently slips through). Suggest adding: exactly-64 vs 65 verdicts; a control char in an id/path/note; a malformed decidedAt; a note at 4096/4097; and the 128/129 id boundary from issue #2.

Verdict

COMMENT — no blockers for the packaging/layout path (constraint 1 is clean and the CJK gate is unaffected). Recommend addressing issues #1 (unenforced "every criterion" invariant — it defeats the purpose of the record) and #2 (dead 128 bound) before building the space UI on top. Issues #3–#7 are worth fixing while the contract is still unreleased and cheap to change.