Repository navigation
feat: scoring gates, reflexion loops, and side-effect classification - #16
Merged
Merged
Conversation
The Top 3 features from the 2026-07 frontier council, implemented per the
arbitrated plan (docs/internal/plan-2026-07-top3.md), in the arbiter's
order: idempotent → score → reflexion.
## Feature C — Side-effect classification (`idempotent: false`)
Any phase may declare it has irreversible side effects (webhook POSTs,
deploys, DB writes). When set:
- transient provider errors are NOT auto-retried — repeating a
side-effecting call behind the author's back is exactly the hazard the
flag prevents; explicit `retry{}` stays honored (the author's declared
acceptance of repeats)
- the result is never cached in ANY scope: within-run resume, cross-run
serve AND store, map per-item — one cacheScope="off" assignment gates
every path, including under a flow-level `incremental`
- the phase state records `sideEffect: true` (rendered as ⚡), stamped at
executePhase's single exit point so every type branch is covered
Default true — 100% backward compatible. Validation warns on the
cross-run/incremental conflicts (runtime silently does the safe thing).
## Feature A — Scoring gates (gate `score` field)
Gates gain graded, composable, auditable quality checks. Six deterministic
scorers (exact-match, contains, regex[negate], json-schema — reusing the
expect contract validator, length-range, code-compiles via node --check /
tsc) run against an interpolated target at ZERO tokens, combined via
all/any/weighted+threshold:
- deterministic pass → auto-PASS, no LLM call (mirrors the eval fast-path;
with weighted+judge the deterministic score is a LOWER BOUND — clearing
the threshold makes the judge's opinion irrelevant, so it is skipped)
- deterministic fail → optional llm-judge decides (sees target + scorer
report; fail-open PASS on unparseable output), or the gate task runs
with the report appended, or — with no fallback — explicit BLOCK
(a deterministic failure is not ambiguity)
- structured result is the gate's ps.json: downstream phases route on
{steps.<gate>.json.combined} / .json.results.N.passed
- score gates support onBlock:"retry" (re-run upstream, re-score) with the
same depth/budget guards as the non-score path
- malformed score fails open to the plain LLM gate with a warning
Pure/impure split mirrors contract.ts: scorers.ts (pure, zero-dep) ↔
scorer-runtime.ts (spawns compilers, 30s timeout, temp-file cleanup).
## Feature B — Reflexion memory in loops (`reflexion: true`)
Loop iterations after the first receive a structured failure summary of
the prior one via {reflexion} (auto-appended when absent, with a one-time
warning; iteration 1 sees a sentinel): expect-contract diagnostics (the
strongest signal — "$.done: required key is missing"), the error message,
or the unmet until condition, plus a truncated output snippet (2000-char
cap, head+tail truncation).
Semantics shift to enable self-correction: body failures become FEEDBACK
instead of terminating the loop. Timeout/abort still hard-stop; exhausting
maxIterations on a failure still fails the phase (reflexion defers
failure, never erases it). Convergence/until still evaluate on successful
outputs only. The last summary is persisted on ps.loop.reflexion for
audit. reflexion off (default) is byte-for-byte the historical behavior.
Also fixes a latent validation bug: loop phases legitimately reference
their own output ({steps.<thisId>.json.done} in until — the documented
stop-condition pattern) but the self-reference check flagged it as an
error; loops are now exempt.
## Surface updates
- schema.ts: 3 new PhaseSchema fields + validation + collectRefs scanning
score.target / score.judge.task
- interpolate.ts: {reflexion} placeholder (single string, no sub-paths)
- store.ts: PhaseState.sideEffect, gate.scores, loop.reflexion
- render.ts: ⚡ side-effect marker, combined score next to PASS/BLOCK
- skills (pi SKILL.md + codex plugin SKILL.md), README, and a runnable
examples/quality-pipeline.json composing all three features
- 49 new tests (11 idempotent + 21 score + 17 reflexion); suite 1001/1001
Ships docs/internal/plan-2026-07-top3.md — the arbitrated implementation
plan this PR executes.
Multi-agent adversarial review (4 attack lenses → code-grounded claims verifier → final arbiter; run top3-adversarial-review-mr4drcvh) ruled MERGE-WITH-FIXES on PR #16. This commit applies all 3 merge blockers and all 5 should-fixes. Merge blockers: - C1 (correctness): all/any + judge no longer auto-skips the judge when deterministics pass. The auto-pass fast-path now requires that the judge CANNOT veto: either no judge is configured, or combine is weighted (where the deterministic score is a judge-inflated lower bound). With all/any the judge's verdict is authoritative — it may check what scorers cannot (e.g. factuality) — so it always runs. Test: "all/any + judge — judge ALWAYS runs". - S1 (security): code-compiles scorers are blocked in dynamic (LLM-generated) sub-flows. The typescript path runs `npx --no-install tsc`, resolving node_modules/.bin from the working directory — a repo-planted fake tsc becomes arbitrary code execution, the same capability class as the already-blocked `script` phases. - S2 (security): regex scorers are blocked in dynamic sub-flows — `new RegExp(pattern).test(target)` with an LLM-authored catastrophic- backtracking pattern is a synchronous ReDoS with no timeout. Author-written flows keep both (a human reviewed them). Test: "dynamic flows: code-compiles and regex scorers are blocked". Should-fixes: - R2: reflexion loops now hard-stop on over-budget (previously only the runOne retry loop and run-level guard applied; a failing reflexion loop could spend past the ceiling). Also folds cumulative loop usage into the live phase state each iteration so overBudget() actually sees mid-loop accrual. Test: "over-budget hard-stops the loop". - S4: provider error noise is sanitized (sanitizeErrorMessage) before reflexion injects it into the next prompt — HTML gateway pages no longer pollute iteration prompts. Contract-violation messages are passed through verbatim (they're runtime-generated, and the precise diagnostics are the feature). Test: "provider error noise is sanitized before prompt injection". - S5: the judge prompt neutralizes ``` fences in the model-produced target (zero-width-space split) so crafted output cannot close the evidence block and inject instructions at prompt level. Test: "judge prompt neutralizes fences in the target". - V5: until/maxIterations/convergence on non-loop phases now warn (previously silently ignored). - V1: per-scorer fields not applicable to the scorer's type are now shape errors (e.g. {type:"contains", negate:true} was silently dropping negate — the author believed negation happened). Docs updated to match: SKILL.md (pi + codex) and README now state the judge-cannot-veto auto-pass condition and the dynamic-flow scorer blocks. Suite: 1006/1006 (5 new regression tests pinning the fixes).
This was referenced Jul 3, 2026
Owner
Author
🔍 Multi-agent adversarial review — ruling appliedProcess: 4 parallel attack lenses (risk-reviewer: runtime semantics · security-reviewer: new attack surface · reviewer: DSL/docs consistency · critic: design decisions) → code-grounded claims verifier (every finding checked against actual source; hallucinated findings refuted) → final-arbiter ruling. Verdict: MERGE-WITH-FIXES — all 3 blockers + all 5 should-fixes applied in d63ed41. Merge blockers (fixed)
Should-fixes (applied)
Notable refuted findings (verifier caught these hallucinations)
Deferred → filed as issues#17 (reflexion failure history) · #18 (temp-file TOCTOU + process-group kill) · #19 (interpolation docs + codex SKILL parity) · #20 (idempotent resume warning + approval/flow semantics) Design rulings (arbiter, kept as-is)
Suite: 1006/1006 (5 new regression tests pin the fixes). |
…ve + close follow-ups Integrates the single-source skill architecture (#14) with the Top 3 features (#16): the score/reflexion/idempotent documentation, previously edited directly in the generated SKILL.md files, is now authored ONCE in skills-src/taskflow/core.md and compiled to both hosts via build-skills.mjs. The drift guard (skills-build.test.ts) passes. Also resolves all four deferred review follow-ups (issues #17-#20) so the release ships without known debt: - #17 (loop): PhaseState.loop.failures records every failed iteration (bounded to 20, sanitized). A reflexion loop that continues past failures no longer loses the intermediate diagnostics — only the terminal one previously survived in `error`. - #18 (scorer-runtime): code-compiles now runs inside an isolated mkdtempSync directory used as the compiler cwd. Closes the predictable temp-name TOCTOU race AND stops `npx tsc` resolving a repo-planted node_modules/.bin/tsc from the phase cwd (defense-in-depth beyond the dynamic-flow scorer block). The cwd parameter is dropped from the scorer signature. (Process-group kill declined: the codebase's script phase kills the direct child too; orphans are bounded by the 30s timeout and cross-platform process groups would diverge from that.) - #19 (docs): the shared core.md now documents {reflexion}, {loop.*}, and the score.target/judge.task interpolation sites; codex SKILL.md reaches feature parity automatically through the single-source build. - #20 (idempotent): a non-idempotent phase that re-executes on resume (a prior attempt had completed) now surfaces a warning so operators see the side effect re-firing; validation warns that idempotent:false on approval/flow phases is a no-op. Docs (README, both SKILL.md sets) reflect the post-review score semantics: auto-pass only when the judge cannot veto (no judge, or weighted lower-bound), and the dynamic-flow code-compiles/regex blocks. Suite: 1014/1014 (drift guard + 4 new regression tests for the follow-ups). dist rebuilt.
…DoS) CodeQL flagged 2 high-severity js/polynomial-redos alerts on PR #16. Root cause: the #16 edit to interpolate.ts (the {reflexion} placeholder hook) shifted line numbers, re-attributing two PRE-EXISTING super-linear regexes in safeParse to the PR diff. Both are genuine polynomial backtracking and worth fixing regardless of attribution: - The fence extractor had an OPTIONAL trailing newline, letting the horizontal-whitespace class and the body's lazy [\s\S]*? both match a whitespace run — quadratic on a fence opener followed by many spaces/tabs with no closing fence. Rewritten so the info string stops at the first newline (no overlap with the body) and the newline is mandatory — linear by construction. (scorer-runtime's extractCode already used a mandatory newline, which is why CodeQL did not flag it.) - The stray-key hint had two \s* around an optional separator (ambiguous whitespace). Collapsed to a single character class — linear. Behavior preserved (all fence/stray-key tests pass unchanged; every fenced example in the suite uses a newline after the tag). Adds a regression test asserting safeParse stays sub-second on 100k-char adversarial inputs. Suite: 1015/1015.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The Top 3 features from the 2026-07 frontier council, in the arbiter's order (C → A → B), per the arbitrated plan in
docs/internal/plan-2026-07-top3.md(included).idempotent: false— side-effect classificationincrementalusage; prerequisite for checkpoint snapshotsscore— scoring gates (6 deterministic scorers + llm-judge, all/any/weighted)reflexion: true— reflexion memory in loopsDesign highlights
C (2 surgical lines + a single-exit-point stamp):
cacheScope = "off"gates ALL cache paths at once (within-run, cross-run serve/store, map per-item,incremental)retry{}stays honored (the author's declared acceptance of repeats)sideEffect: truemarker (⚡ in the TUI), stamped in theexecutePhasewrapper so all 10 type branches are coveredA (extends
gate— no 11th phase type):evalfast-path)score→ plain LLM gate. Deterministic failure with no fallback → explicit BLOCK (not ambiguity).json— downstreamwhen: "{steps.quality.json.combined} >= 0.9"routing workscontract.ts:scorers.ts(pure) ↔scorer-runtime.ts(spawns compilers, 30s cap)B (arbiter-resolved semantics):
expectcontract diagnostics ($.done: required key is missing) feed the next iteration verbatimmaxIterationsexhausted on a failure still fails the phase (reflexion defers, never erases)reflexionoff = byte-for-byte historical behavior (pinned by test)untilself-references ({steps.<thisId>.json.done}— the documented pattern) were flagged as errorsCross-feature composition (verified end-to-end)
examples/quality-pipeline.json: analyze → score gate (weighted, judge escalation) → reflexion loop (contract-driven action plan) → idempotent:false notify script. A mock-runner smoke test of this exact composition passes: zero-token gate auto-pass, iteration-2 prompt carries the$.donediagnostic, notify records ⚡ and never caches.Test plan
npm run typecheck+npm run buildclean