Skip to content

refactor(autoimplement): apply the review polish on the prompt evidence work - #101

Merged
osolmaz merged 1 commit into
mainfrom
fix/prompt-evidence-review-polish
Sep 18, 2026
Merged

osolmaz merged 1 commit into
mainfrom
fix/prompt-evidence-review-polish

Conversation

@osolmaz

@osolmaz osolmaz commented Sep 18, 2026

Copy link
Copy Markdown
Owner

Summary

The bounded-evidence change merged in #100 was correct, and a Fable review found five small ways to
improve it.
Four are now applied, and the fifth is recorded as an accepted limit.
None of them fixes a defect that could break a run.

The largest one is a small waste: the decide prompt listed one control decision twice, because the
dispatch step returns the decision the decide step already recorded.
The rest are an inaccurate code comment, a dead helper, and two documentation corrections.

What Changed

The prompt already deduplicates results, but it missed the dispatch step.
A control turn records decide (the decision) and then dispatch (the same decision, returned
unchanged), so both copies reached the ledger.
The ledger now skips the steps whose output the prompt shows elsewhere, in one named set.

  • src/builtins/autoimplement.workflow.ts: PROMPT_ECHO_STEP_NODES replaces the single observe
    constant and holds observe and dispatch. The ledger skips both, plus the include return steps it
    already skipped.
  • src/workflows/prompt-evidence.ts: the EvidenceRef.digest comment said a digest lets a reader
    locate the durable record. That holds for a reference built from a recorded subtree, but the ledger
    and observation bounds digest a projected value, which matches no stored blob. The comment now says
    what the digest covers.
  • src/workflows/prompt-evidence.ts: deleted describeValue, which returned typeof value in every
    branch. refSource uses typeof value directly.
  • docs/WORKFLOWS.md: the ceiling section said the ceiling keeps room for the live-control block that
    the engine appends. No code reserves that room. The section now states the real relation: one
    complete request can be about 8,192 characters plus the step contract larger than the ceiling, and
    that headroom is far below the context window the ceiling protects.
  • docs/plans/2026-09-17-prompt-evidence-budget-plan.md: records two accepted limits and corrects the
    branch coverage figure from 92.75% to 94.38%.

The two recorded limits are deliberate:

  • The Plan: line stays whole, because it is the decider's main input. A plan near the ceiling makes
    the decide node fail with a named error, and the controller blocks after three failures.
  • The observation shortens field by field and never becomes one reference. Replacing the whole
    observation would also drop availableRoutes and the other small decisive fields, and a decider
    without the routes is worse than a named failure that reports the sizes.

Testing

I ran the full local suite and one real-model live E2E.

The new ledger case uses the real include node id shapes and a realistic step order, so the fixture
can occur in a real run.
It fails on the earlier code: the decision reason appears twice without the fix.

  • npm run check passes: format, lint, typecheck, build, tests, coverage.
  • npm run test:e2e passes (14 tests).
  • npx slophammer-ts@latest dry . reports 0 candidates.
  • npx slophammer-ts@latest check . --only ts.dependency-boundaries-required reports no findings.
  • Live E2E: deepseek/deepseek-v4-flash through openrouter, result: passed, measured model cost
    $0.0024.

Not tested: the prompt builders of the other built-in workflows still embed raw command output. That
work is unchanged and still open.

Risks

Risk is low. The change touches a prompt-only filter, one comment, and documentation.

  • The ledger drops the dispatch step, which carries no evidence the decide step does not carry.
    A test asserts that the decision appears once and that the include and observation steps stay out.
  • Projection still runs only when a prompt is built, so recorded runs stay readable and resumable.
  • The deleted helper had one call site, and its behavior is unchanged.

Follow-ups

The other Autoimplement prompt sites (review, CI, and comment inspection) still embed command output,
which the command-batch limit allows up to 1,000,000 characters per check.
Those results carry no log file path, so bounding them needs a decision about how an agent reads the
full log afterwards.

…ce work

A Fable review of the merged change found no defect and five small improvements.
This applies four of them and documents the fifth.

- The `EvidenceRef` digest comment claimed that a digest locates the durable
  record. That holds for a reference built from a recorded subtree, but the
  ledger and observation bounds digest a projected value, which matches no
  stored blob. Say what the digest covers.
- The ledger kept the `dispatch` step, which returns the decision the `decide`
  step already recorded, so one control turn listed the same decision twice.
  Skip the steps whose output the prompt already shows in one named set.
- `describeValue` returned `typeof value` in every branch, so delete it.
- `docs/WORKFLOWS.md` said the ceiling keeps room for the engine's appended
  block. No code reserves that room, so state the real relation instead.
- Record two accepted limits in the plan: the `Plan:` line stays whole, and the
  observation shortens field by field rather than as one value, because that
  would drop the available routes. Correct the branch coverage figure.

Co-Authored-By: DeepSeek V4.1 Flash · Auto · Novita <noreply@pi.dev>
Generated-By: pi 0.85.1 (https://pi.dev)
@osolmaz
osolmaz merged commit 688d299 into main Sep 18, 2026
4 checks passed
@osolmaz
osolmaz deleted the fix/prompt-evidence-review-polish branch September 18, 2026 04:42
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.

1 participant