PR #2 needs changes before merge

Changes requested

6 P1 findings · 1 P2 finding · 223 baseline tests pass

10 required-behavior cases fail · valid-request control passes

+1531 / -84 91c123b

The accepted-turn bridge is the right architectural direction, but I would request changes at 91c123b. Six blocking findings and one smaller identity inconsistency remain. They are reproduced below, with ten failing required-behavior cases and one passing valid-request control.

Reviewed on 22 September 2026 against merged base 3d3bf9b, through exact head 91c123b06b177c6b943fe2ac2f0ffd1e8c2f9893. This is a review of PR #2, including its latest corrective commit. The main workspace remains unchanged. Tests ran in an isolated worktree.

Assessment

I would keep the frame-to-turn-to-command architecture. Recording accepted judgments, checking capability citations and applying exclusions before execution are useful responsibilities for this layer. There is no need to add English rules or replace the transaction kernel to resolve these findings.

The implementation has two partially independent validators. One rejects queries but ignores malformed sibling exclusions. The other rejects malformed siblings but now executes queries. This is the immediate architectural weakness: a caller's choice of API changes the safety contract. The latest refactor removed a call to the older helper and accidentally removed its act/operation checks from the new path.

The second boundary problem crosses packages. The compiler claims to cover refs in arguments, but the kernel drops their evidence unless those refs also occupy slots. Adding more compiler checks cannot compensate for a consumer that discards the certified read set.

Use one validated whole-turn representation and a narrow command constructor. Either preserve command-wide read dependencies through the kernel or refuse unsupported argument refs. Rejected prerequisites must not leave independently executable dependents behind. These are bounded repairs; a larger framework would not address the demonstrated failures.

Verification and test quality

pnpm test passes all 223 tests across 19 eval files, and pnpm typecheck passes. The PR description's 202-test figure is stale. The independent probe suite has ten assertion failures covering seven findings; its valid-request control passes. The probe file also passes the repository's strict TypeScript settings.

The query test already in the repository cites a referent decision, so capability-citation rejection makes it pass without testing the act guard. The independent query cases supply valid capability evidence, and each reaches an actual executor invocation. Evidence coverage is tested across compiler and kernel, including an authority that would reject a stale argument ref if that ref reached validation.

The difficult-utterance E2E test exercises a useful contract: exclusion of B, action on A and preservation of frame versions. Its backend answers and propositions are scripted. It is not evidence that the implementation can interpret arbitrary wording, and that remains a legitimate boundary for this PR.

Next step

Fix F31-F36 before merging. F37 is a smaller provenance inconsistency worth fixing in the same shared validation pass. Keep all 223 existing tests passing, retain these required-behavior probes, and rerun them against the final PR head. No GitHub comment, formal approval or source change was submitted by this review.

cd /tmp/circe-pr2-review
pnpm test
pnpm typecheck
pnpm exec vitest run --config /tmp/circe-pr2-evidence/vitest.config.mjs --reporter=verbose

The downloaded test and config use local worktree paths. Adjust aliases for another checkout. Finding numbers continue from the earlier safety review; PR #1's scoped approval is unchanged.

Implementation walkthrough
Workflow run → accepted decision ids + traces → whole-turn validation → evidence-bound commands → kernel validation/freeze → executor
Changed files

Findings

Required-behavior tests Vitest config Probe output Baseline output Typecheck output Review sidecar

Prompt for all findings · Ctrl+`
Prompt to rerun review · Ctrl+Space
P1 F34. A dangling prohibition is silently ignoredpackages/perception/src/proposals.ts:123-135logic correctness
shapeIssue checks that a relation targetIndex is an integer, but does not check its bounds. Bounds are checked later in relationIssue, after exclusions have already returned from the compilation loop. The whole-turn malformed-input rule consequently misses dangling relations, including prohibition-only propositions.


The reproduction submits an exclusion with no direct refs and an excludes relation targeting index 99 in a two-proposition turn. The exclusion is labeled an exclusion constraint, while the other proposition compiles into a command. The malformed prohibition was never validated or rejected as malformed.


The PR expressly promises that malformed relations make the whole turn uncompilable because ignoring them can discard intended prohibitions. The current test covers a string index, but an out-of-range integer passes that shape gate.
Fix direction. Validate relation bounds and target existence for the entire turn before analyzing exclusions or constructing any commands. Apply the same validation to exclusion-only propositions and both public entry points.
Source at reviewed head · Probe R4
Captured source
123:   if (proposition.relations !== undefined) {
124:     if (!Array.isArray(proposition.relations)) {
125:       return "malformed proposition: relations is not an array";
126:     }
127:     for (const relation of proposition.relations) {
128:       if (
129:         !isRecord(relation) ||
130:         typeof relation.kind !== "string" ||
131:         !RELATION_KINDS.has(relation.kind) ||
132:         !Number.isInteger(relation.targetIndex)
133:       ) {
134:         return "malformed proposition: relation is not a typed, integer-indexed reference";
135:       }
Prompt for AI agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate.

In `packages/perception/src/proposals.ts` around lines 123-135, shapeIssue checks that a relation targetIndex is an integer, but does not check its bounds. Bounds are checked later in relationIssue, after exclusions have already returned from the compilation loop. The whole-turn malformed-input rule consequently misses dangling relations, including prohibition-only propositions. Validate relation bounds and target existence for the entire turn before analyzing exclusions or constructing any commands. Apply the same validation to exclusion-only propositions and both public entry points. Reproduce R4 using `/tmp/circe-pr2-evidence/probes.test.ts` at PR #2 head 91c123b06b177c6b943fe2ac2f0ffd1e8c2f9893. Keep valid requests working and run the full suite and typecheck after the change.
P1 F33. Rejected prerequisites do not block dependent commandspackages/perception/src/proposals.ts:355-359logic correctness
relationIssue treats a dependsOn target as satisfied when its cited decisions are accepted. It never checks whether that proposition survives evidence validation, prohibitions or other compilation checks. compileSemanticTurn then returns the dependent command with no representation of the relation.


The reproduction has proposition 0 request B without a frame observation, and proposition 1 request A with dependsOn:0. The compiler explicitly rejects proposition 0 for missing evidence yet emits proposition 1. A caller executing the returned commands cannot discover the missing prerequisite from that command.


Acceptance of a judgment is weaker than acceptance of the proposition it supports. A whole-turn compiler needs to preserve that distinction. This finding concerns a rejected prerequisite at compilation; it does not require adding a general workflow scheduler to this PR.
Fix direction. Compute proposition validity before emitting commands and propagate rejection through dependency edges. Reject executable dependency forms that the command contract cannot preserve; define semantic-only dependencies explicitly rather than silently treating rejected requests as satisfied.
Source at reviewed head · Probe R3
Captured source
355:     const related = turn.propositions[relation.targetIndex];
356:     if (!related) return "malformed proposition: relation target is missing";
357:     if (relation.kind === "dependsOn") {
358:       if (!isAccepted(related)) return "a dependency proposition was not accepted";
359:     }
Prompt for AI agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate.

In `packages/perception/src/proposals.ts` around lines 355-359, relationIssue treats a dependsOn target as satisfied when its cited decisions are accepted. It never checks whether that proposition survives evidence validation, prohibitions or other compilation checks. compileSemanticTurn then returns the dependent command with no representation of the relation. Compute proposition validity before emitting commands and propagate rejection through dependency edges. Reject executable dependency forms that the command contract cannot preserve; define semantic-only dependencies explicitly rather than silently treating rejected requests as satisfied. Reproduce R3 using `/tmp/circe-pr2-evidence/probes.test.ts` at PR #2 head 91c123b06b177c6b943fe2ac2f0ffd1e8c2f9893. Keep valid requests working and run the full suite and typecheck after the change.
P1 F35. The explicit proposition index can bypass prohibitionspackages/perception/src/proposals.ts:453-469logic correctness
The new index option is trusted without checking its range or confirming that turn.propositions[index] matches the supplied proposition. The code uses that index to look up turn-wide prohibitions, while it constructs the command from the separate proposition argument.


The reproduction places a relation-only exclusion at index 0 prohibiting the request at index 1. Calling propositionCommand with that request and index:0 returns an executable command. Checking the correct position would reject it. Rehydration or filtering code can therefore make the guard inspect one proposition while executing another.


This is a defect in the new exported low-level API even though compileSemanticTurn no longer calls it. The API explicitly exists for callers holding accepted decisions and advertises the same turn-wide prohibitions.
Fix direction. Require an integer index within bounds and verify identity or canonical structural equality with the selected turn entry. Reject mismatches before reading prohibitions, or accept only the turn and index and resolve the proposition internally.
Source at reviewed head · Probe R5
Captured source
453:   // Prefer an explicit position; fall back to identity, then to a unique
454:   // structural match so rehydrated turns are not silently dropped.
455:   let index = options.index ?? turn.propositions.indexOf(proposition);
456:   if (index < 0) {
457:     const serialized = stableStringify(proposition);
458:     const matches = turn.propositions
459:       .map((candidate, candidateIndex) => ({ candidate, candidateIndex }))
460:       .filter(({ candidate }) => stableStringify(candidate) === serialized);
461:     if (matches.length === 1) index = matches[0]!.candidateIndex;
462:   }
463:   if (index < 0) return undefined;
464:
465:   const issues = turn.propositions.map((candidate) => shapeIssue(candidate));
466:   const isAccepted = (candidate: SemanticProposition): boolean =>
467:     decisionsAccepted(candidate, options.acceptedDecisions);
468:   const analysis = analyzeTurn(turn, isAccepted, issues);
469:   if (analysis.prohibited.has(index) || analysis.unvalidatedProhibited.has(index)) {
Prompt for AI agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate.

In `packages/perception/src/proposals.ts` around lines 453-469, The new index option is trusted without checking its range or confirming that turn.propositions[index] matches the supplied proposition. The code uses that index to look up turn-wide prohibitions, while it constructs the command from the separate proposition argument. Require an integer index within bounds and verify identity or canonical structural equality with the selected turn entry. Reject mismatches before reading prohibitions, or accept only the turn and index and resolve the proposition internally. Reproduce R5 using `/tmp/circe-pr2-evidence/probes.test.ts` at PR #2 head 91c123b06b177c6b943fe2ac2f0ffd1e8c2f9893. Keep valid requests working and run the full suite and typecheck after the change.
P1 F36. The per-proposition API drops malformed exclusionspackages/perception/src/proposals.ts:465-468logic correctness
propositionCommand computes shape issues for every proposition but never rejects a malformed turn. analyzeTurn skips any proposition with a shape issue. In contrast, compileSemanticTurn rejects the whole turn before this step.


The reproduction supplies an exclusion of A whose relations field is malformed, alongside a valid request for A. The whole-turn API correctly rejects it. The per-proposition API skips the malformed exclusion and emits the request for A, complete with current version evidence.


The unvalidated-exclusion sets do not help here because the malformed proposition is skipped before any refs enter those sets. An API promising fail-closed exclusions must not turn a malformed constraint into permission.
Fix direction. Apply one whole-turn structural validation pass to both entry points. A malformed sibling must prevent command construction, or make the low-level helper internal to an already validated turn.
Source at reviewed head · Probe R6
Captured source
465:   const issues = turn.propositions.map((candidate) => shapeIssue(candidate));
466:   const isAccepted = (candidate: SemanticProposition): boolean =>
467:     decisionsAccepted(candidate, options.acceptedDecisions);
468:   const analysis = analyzeTurn(turn, isAccepted, issues);
Prompt for AI agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate.

In `packages/perception/src/proposals.ts` around lines 465-468, propositionCommand computes shape issues for every proposition but never rejects a malformed turn. analyzeTurn skips any proposition with a shape issue. In contrast, compileSemanticTurn rejects the whole turn before this step. Apply one whole-turn structural validation pass to both entry points. A malformed sibling must prevent command construction, or make the low-level helper internal to an already validated turn. Reproduce R6 using `/tmp/circe-pr2-evidence/probes.test.ts` at PR #2 head 91c123b06b177c6b943fe2ac2f0ffd1e8c2f9893. Keep valid requests working and run the full suite and typecheck after the change.
P1 F31. The whole-turn compiler turns queries into mutationspackages/perception/src/proposals.ts:605-616logic correctness
compileSemanticTurn no longer calls propositionCommand. It calls buildCommand directly after citation and relation checks, but never checks that the act is request or that the operation is reference or slot-fill. Those checks exist only in the other API at lines 445-449. The latest 91c123b refactor removed their effective use by the whole-turn compiler.


A resolved query/inspect proposition with an accepted task.resume capability citation and an observed target compiles to a propose command and reaches the executor. The same happens for query/reference and request/compose. All three required-behavior tests fail on an actual invocation, not merely on command construction.


The existing query regression supplies a referent citation rather than the capability citation needed to get this far, so it passes for a different rejection reason. The kernel receives no dialogue act and cannot repair the lost distinction.
Fix direction. Use one executable-proposition validator for both entry points, including capability presence and act/operation eligibility. Test non-executable acts with otherwise valid citations and evidence.
Source at reviewed head · Probe R1
Captured source
605:     const issue = citationIssue(proposition, turn, acceptedDecisions, tracesById, tracesAvailable);
606:     if (issue) {
607:       rejected.push({ index, reason: issue });
608:       return;
609:     }
610:     const relation = relationIssue(turn, proposition, index, accepted);
611:     if (relation) {
612:       rejected.push({ index, reason: relation });
613:       return;
614:     }
615:
616:     const declared = slotsOf(proposition);
Prompt for AI agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate.

In `packages/perception/src/proposals.ts` around lines 605-616, compileSemanticTurn no longer calls propositionCommand. It calls buildCommand directly after citation and relation checks, but never checks that the act is request or that the operation is reference or slot-fill. Those checks exist only in the other API at lines 445-449. The latest 91c123b refactor removed their effective use by the whole-turn compiler. Use one executable-proposition validator for both entry points, including capability presence and act/operation eligibility. Test non-executable acts with otherwise valid citations and evidence. Reproduce R1 using `/tmp/circe-pr2-evidence/probes.test.ts` at PR #2 head 91c123b06b177c6b943fe2ac2f0ffd1e8c2f9893. Keep valid requests working and run the full suite and typecheck after the change.
P1 F32. Argument reference evidence disappears at the kernel boundarypackages/perception/src/proposals.ts:665-676logic data-integrity
The new argument-reference path adds versions for argument-only refs to command.expectedVersions. However, the kernel in runtime.ts:1051-1057 filters that list to refs present in named slot bindings. Its commit path validates and freezes only binding expectations. The new compiler therefore emits evidence that its actual consumer silently drops.


The reproduction compiles slot target A plus arguments.secondary = B. The command correctly includes versions for A and B, but authority validation receives only A and the executor receives the B argument without a B precondition. A second test makes authority validation return stale for B if asked. Execution still happens because B is never checked.


This breaks the newly claimed coverage of every executable argument reference. Seeing B in the frame at compile time does not establish that B is current at dispatch. No real backend or persistence adapter is required to reproduce the gap.
Fix direction. Until command-wide read preconditions survive validation and freezing, reject argument refs outside executable bindings. Alternatively add explicit command-level read dependencies and preserve them through every effect unit; do not silently invent target slots that change execution semantics.
Source at reviewed head · Probe R2 / R2b
Captured source
665:     // Every executable ref must carry the version the interpretation read.
666:     const evidence = expectedVersionsFromFrame(options.frame, [
667:       ...constrained.flatMap((slot) => slot.refs),
668:       ...argumentRefs,
669:     ]);
670:     for (const ref of [...constrained.flatMap((slot) => slot.refs), ...argumentRefs]) {
671:       if (!evidence.some((precondition) => entityRefKey(precondition.ref) === entityRefKey(ref))) {
672:         rejected.push({ index, reason: `no frame observation for ${entityRefKey(ref)}` });
673:         return;
674:       }
675:     }
676:     const command = buildCommand({ proposition, commandId, slots: constrained, argumentRefs, evidence });
Prompt for AI agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate.

In `packages/perception/src/proposals.ts` around lines 665-676, The new argument-reference path adds versions for argument-only refs to command.expectedVersions. However, the kernel in runtime.ts:1051-1057 filters that list to refs present in named slot bindings. Its commit path validates and freezes only binding expectations. The new compiler therefore emits evidence that its actual consumer silently drops. Until command-wide read preconditions survive validation and freezing, reject argument refs outside executable bindings. Alternatively add explicit command-level read dependencies and preserve them through every effect unit; do not silently invent target slots that change execution semantics. Reproduce R2 / R2b using `/tmp/circe-pr2-evidence/probes.test.ts` at PR #2 head 91c123b06b177c6b943fe2ac2f0ffd1e8c2f9893. Keep valid requests working and run the full suite and typecheck after the change.
P2 F37. Frame binding checks content but omits identitypackages/perception/src/proposals.ts:533-550logic data-integrity
The whole-turn compiler checks frame.hash against turn.frameHash but does not compare frame.id with turn.frameId. propositionCommand checks both. Before the latest refactor the whole-turn compiler reached that identity check through the helper; it no longer does.


The reproduction supplies the same content and hash under a different frame id. The turn still compiles. This is a provenance and contract inconsistency; the probe does not demonstrate different world content or an unsafe mutation, so it is lower severity than the execution defects.


The accepted-turn contract and PR description record and promise both frame id and hash. Enforcing one of them in only one API makes the evidence-binding behavior depend on which compiler the caller chooses.
Fix direction. Require both frame id and hash to match in the shared validation path. If content equivalence is intentionally sufficient, make that explicit across the contract and both APIs instead of silently changing it in one.
Source at reviewed head · Probe R7
Captured source
533:   // Evidence must come from the frame the turn was decided against.
534:   if (options.frame && turn.frameHash === undefined) {
535:     return {
536:       commands: [],
537:       rejected: turn.propositions.map((_, index) => ({
538:         index,
539:         reason: "turn has no frame identity to bind the supplied frame to",
540:       })),
541:     };
542:   }
543:   if (options.frame && turn.frameHash !== undefined && options.frame.hash !== turn.frameHash) {
544:     return {
545:       commands: [],
546:       rejected: turn.propositions.map((_, index) => ({
547:         index,
548:         reason: "frame does not match the frame the turn was decided against",
549:       })),
550:     };
Prompt for AI agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate.

In `packages/perception/src/proposals.ts` around lines 533-550, The whole-turn compiler checks frame.hash against turn.frameHash but does not compare frame.id with turn.frameId. propositionCommand checks both. Before the latest refactor the whole-turn compiler reached that identity check through the helper; it no longer does. Require both frame id and hash to match in the shared validation path. If content equivalence is intentionally sufficient, make that explicit across the contract and both APIs instead of silently changing it in one. Reproduce R7 using `/tmp/circe-pr2-evidence/probes.test.ts` at PR #2 head 91c123b06b177c6b943fe2ac2f0ffd1e8c2f9893. Keep valid requests working and run the full suite and typecheck after the change.