Skip to content

Implement the ADR 002 deterministic-tiered review route (7.1-7.4, 7.6.1)#6

Merged
leynos merged 56 commits into
mainfrom
api-key-support
Jul 24, 2026
Merged

Implement the ADR 002 deterministic-tiered review route (7.1-7.4, 7.6.1)#6
leynos merged 56 commits into
mainfrom
api-key-support

Conversation

@leynos

@leynos leynos commented Jul 18, 2026

Copy link
Copy Markdown
Owner

Summary

This branch implements
ADR 002:
the deterministic-tiered review route with hard cost admission control,
live-validated end to end. Deterministic host code absorbs every derivable
phase (configuration, range preparation, report rendering, history
recording — four model calls deleted); the remaining judgement runs on two
OpenAI Flex lanes (gpt-5.6-luna finder packs, one gpt-5.6-terra
issue-set audit replacing the thirty-call per-candidate verifier fan-out)
behind a reserve-first admission controller with a versioned pricing table
and per-call ledger. Every live corpus review, including a 101-file
oversize probe, came in below USD 0.25 reported (range 0.021-0.115); a
seeded-defect review scored 3/3 recall with zero false positives.

Roadmap tasks: (7.1.1), (7.1.2), (7.1.3), (7.2.1), (7.2.2), (7.2.3),
(7.3.1), (7.3.2), (7.3.3), (7.3.4), (7.4.1), (7.4.2), (7.6.1)

Execplan:
docs/execplans/api-key-support.md
— implemented through milestones M0-M7 plus the M8 gate-enablement
addendum, with the live ledger, hand assessments, decision log, and
retrospective maintained throughout. The companion change adopting this
route in df12-build is
leynos/df12-build#64.

Notable mid-flight decision: ADR 002 originally specified Codex CLI Flex
adapters, but the M0 probe proved Codex never transmits service_tier
(three versions capture-tested; upstream issues openai/codex#26604,
openai/codex#31562, and openai/codex#2916 corroborate), so the adapters
use the pi coding agent with a Dakar-owned extension injecting the tier
through pi's documented hook. The ADR carries the amendment.

Review walkthrough

Validation

  • make check: green at every commit (formatting, markdownlint, nixie,
    docstrings, typecheck, ODW dry run, workflow freshness, 230 tests,
    spelling), run through scrutineer agents with teed logs.
  • Every behaviour change landed red-first; the red evidence per milestone
    is catalogued in the execplan.
  • Live validation (total spend ~USD 0.52 of a USD 5.00 cap): six corpus
    reviews across tiers plus one seeded-defect review — 3/3 ground-truth
    recall, zero false positives, correct severity grading; two audit
    rejections on real PRs judged defensible by hand; one organic finding on
    the oversize probe; admission refusals and pack truncation honestly
    recorded; deferred and zero-coverage reviews never record the head.
  • Dogfood: the route reviewed the df12-build branch that adopts it and
    surfaced a real fail-open defect there before that PR was opened.

Notes

  • The legacy-route-final tag preserves the pre-ADR-002 pipeline as the
    comparison arm for roadmap task 7.5.3.
  • Deferred by design (roadmap 7.5): SARIF envelope, deterministic gate
    short-circuit, and the adjudicated legacy comparison; cost tunings
    (raising adapterOverheadTokens toward the measured ~28k first-call
    cache write, restricting pi tools for finder calls) are recorded in the
    execplan retrospective.
  • Runtime prerequisites: pi on PATH and OPENAI_API_KEY; the
    workflow itself remains dependency-free.

References

References

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Sorry @leynos, your pull request is larger than the review limit of 150000 diff characters

@coderabbitai

coderabbitai Bot commented Jul 18, 2026

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 2176c46c-9f1f-4280-b3a9-5973a9ac7ee0

📥 Commits

Reviewing files that changed from the base of the PR and between 9999af8 and b7cc350.

📒 Files selected for processing (7)
  • docs/execplans/api-key-support.md
  • docs/users-guide.md
  • scripts/live-review-harness.mjs
  • src/workflows/dakar-review/prompts.ts
  • tests/live-harness.test.mjs
  • tests/workflow-prompts.test.mjs
  • workflows/dakar-review.js
💤 Files with no reviewable changes (3)
  • src/workflows/dakar-review/prompts.ts
  • tests/workflow-prompts.test.mjs
  • workflows/dakar-review.js

Summary

  • Implements ADR 002’s deterministic-first, tiered review route for roadmap items 7.1–7.4 and 7.6.1.
  • Moves configuration, range preparation, deterministic gates, SARIF/Markdown rendering, and review-history recording into fail-closed host-side CLI logic.
  • Adds bounded OpenAI Flex routing through pi, with pinned Luna finder packs and a single Terra issue-set audit.
  • Introduces versioned pricing, reserve-first admission control, per-call ledgers, deterministic retries/backoff, deferral, audit caps, and coverage safeguards.
  • Adds policy parsing, deterministic gate execution, telemetry capture, live corpus validation, and extensive unit/property/CLI coverage.
  • Adds the ADR 002 design documentation, repository and user/developer guides, documentation index, roadmap updates, and the new API-key-support ExecPlan.
  • Validation reported costs of USD 0.021–0.115 and 3/3 seeded-defect recall with zero false positives.

Testing and follow-up

  • Runtime Flex adapter tests and the 80% docstring coverage gate still require successful execution.
  • Observability remains out of scope and is tracked by issue #2.

Walkthrough

Dakar now runs deterministic-flex-v1 reviews through CLI preparation, bounded Luna Flex finder packs, one Terra Flex audit, deterministic rendering, usage telemetry, budget admission, CLI-owned history recording, and expanded workflow, harness, documentation, and validation support.

Changes

Dakar Flex review

Layer / File(s) Summary
Pi adapter and CLI execution
adapters/pi/*, bin/dakar-review.mjs, scripts/odw-config.mjs, scripts/live-review-harness.mjs, tests/adapter-config.test.mjs, tests/cli.test.mjs, tests/live-harness.test.mjs
Configure Pi Flex lanes, collect usage, forward tuning options, prepare reviews before ODW execution, and enforce validated recording.
Workflow contracts and planning
src/workflows/dakar-review/*, tests/workflow-*.test.mjs
Add bounded configuration, pinned lanes, pricing, retries, admission contracts, audit schemas, candidate compaction, and Flex finder planning.
Admission, audit, and output flow
src/workflows/dakar-review/main.ts, workflows/dakar-review.js
Replace verification, synthesis, and record phases with admitted Luna calls, a consolidated Terra audit, deterministic rendering, ledger metrics, and conditional recording payloads.
Validation and operating documentation
README.md, docs/*, scripts/live-corpus.json, scripts/build-workflow.mjs, Makefile, odw.config.json, .gitignore, LICENSE, package.json
Document and validate the workflow, cost controls, live corpus, generated artefact inputs, adapter state, licence, and type-check coverage.

Sequence Diagram(s)

sequenceDiagram
  participant Operator
  participant DakarCLI
  participant ReviewState
  participant ODW
  participant LunaFlex
  participant TerraFlex
  Operator->>DakarCLI: Start branch review
  DakarCLI->>ReviewState: Prepare range and state
  ReviewState-->>DakarCLI: Return prepared review or skip
  DakarCLI->>ODW: Run with prepared and tuning arguments
  ODW->>LunaFlex: Dispatch admitted finder packs
  LunaFlex-->>ODW: Return candidate findings
  ODW->>TerraFlex: Submit compacted issue set
  TerraFlex-->>ODW: Return audit verdicts
  ODW-->>DakarCLI: Return deterministic report and ledger
  DakarCLI->>ReviewState: Append completed review
Loading

Possibly related PRs

  • leynos/dakar#1 — Modifies the same Makefile validation and workflow CLI surfaces.
  • leynos/dakar#3 — Modifies the same Makefile node --check coverage.
  • leynos/df12-lints#7 — Adds the same ISC licence text and package licence declaration.

Suggested labels: Roadmap

Poem

Luna packs hum, Terra weighs,
Budgets guide the review maze.
Pi sends Flex through midnight air,
Deterministic reports appear.
Heads are logged when reviews are done.

🚥 Pre-merge checks | ✅ 4 | ❌ 16

❌ Failed checks (1 warning, 15 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 65.52% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Testing (Overall) ❓ Inconclusive Repository clone failed, so this custom check could not run with code access. Retry the review run. If this persists, inspect pre-merge custom-check logs for infrastructure or agent runtime failures.
User-Facing Documentation ❓ Inconclusive Repository clone failed, so this custom check could not run with code access. Retry the review run. If this persists, inspect pre-merge custom-check logs for infrastructure or agent runtime failures.
Developer Documentation ❓ Inconclusive Repository clone failed, so this custom check could not run with code access. Retry the review run. If this persists, inspect pre-merge custom-check logs for infrastructure or agent runtime failures.
Module-Level Documentation ❓ Inconclusive Repository clone failed, so this custom check could not run with code access. Retry the review run. If this persists, inspect pre-merge custom-check logs for infrastructure or agent runtime failures.
Testing (Unit And Behavioural) ❓ Inconclusive Repository clone failed, so this custom check could not run with code access. Retry the review run. If this persists, inspect pre-merge custom-check logs for infrastructure or agent runtime failures.
Testing (Property / Proof) ❓ Inconclusive Repository clone failed, so this custom check could not run with code access. Retry the review run. If this persists, inspect pre-merge custom-check logs for infrastructure or agent runtime failures.
Testing (Compile-Time / Ui) ❓ Inconclusive Repository clone failed, so this custom check could not run with code access. Retry the review run. If this persists, inspect pre-merge custom-check logs for infrastructure or agent runtime failures.
Unit Architecture ❓ Inconclusive Repository clone failed, so this custom check could not run with code access. Retry the review run. If this persists, inspect pre-merge custom-check logs for infrastructure or agent runtime failures.
Domain Architecture ❓ Inconclusive Repository clone failed, so this custom check could not run with code access. Retry the review run. If this persists, inspect pre-merge custom-check logs for infrastructure or agent runtime failures.
Observability ❓ Inconclusive Repository clone failed, so this custom check could not run with code access. Retry the review run. If this persists, inspect pre-merge custom-check logs for infrastructure or agent runtime failures.
Security And Privacy ❓ Inconclusive Repository clone failed, so this custom check could not run with code access. Retry the review run. If this persists, inspect pre-merge custom-check logs for infrastructure or agent runtime failures.
Performance And Resource Use ❓ Inconclusive Repository clone failed, so this custom check could not run with code access. Retry the review run. If this persists, inspect pre-merge custom-check logs for infrastructure or agent runtime failures.
Concurrency And State ❓ Inconclusive Repository clone failed, so this custom check could not run with code access. Retry the review run. If this persists, inspect pre-merge custom-check logs for infrastructure or agent runtime failures.
Architectural Complexity And Maintainability ❓ Inconclusive Repository clone failed, so this custom check could not run with code access. Retry the review run. If this persists, inspect pre-merge custom-check logs for infrastructure or agent runtime failures.
Rust Compiler Lint Integrity ❓ Inconclusive Repository clone failed, so this custom check could not run with code access. Retry the review run. If this persists, inspect pre-merge custom-check logs for infrastructure or agent runtime failures.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed Verify the title; it matches the ADR 002 scope and includes the roadmap reference.
Description check ✅ Passed Verify the description; it directly explains the ADR 002 route and its validation.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch api-key-support

Comment @coderabbitai help to get the list of available commands.

@leynos
leynos marked this pull request as ready for review July 18, 2026 19:14

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Sorry @leynos, your pull request is larger than the review limit of 150000 diff characters

@coderabbitai coderabbitai Bot added the Roadmap label Jul 18, 2026
chatgpt-codex-connector[bot]

This comment was marked as resolved.

coderabbitai[bot]

This comment was marked as resolved.

@leynos

leynos commented Jul 19, 2026

Copy link
Copy Markdown
Owner Author

@coderabbitai resume

@coderabbitai

coderabbitai Bot commented Jul 19, 2026

Copy link
Copy Markdown
✅ Action performed

Reviews resumed.

coderabbitai[bot]

This comment was marked as resolved.

Repository owner deleted a comment from coderabbitai Bot Jul 19, 2026
@leynos

leynos commented Jul 19, 2026

Copy link
Copy Markdown
Owner Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 19, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@leynos

This comment was marked as resolved.

@coderabbitai

This comment was marked as resolved.

coderabbitai[bot]

This comment was marked as resolved.

@leynos

leynos commented Jul 19, 2026

Copy link
Copy Markdown
Owner Author

@coderabbitai resume

@coderabbitai

coderabbitai Bot commented Jul 19, 2026

Copy link
Copy Markdown
✅ Action performed

Reviews resumed.

@leynos

leynos commented Jul 19, 2026

Copy link
Copy Markdown
Owner Author

@coderabbitai Have the following now been resolved?

src/workflows/dakar-review/main.ts (1)

676-685: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Record the models that actually executed.
recordInput.models uses legacy REVIEW_MODELS, while execution uses the Luna and Terra Flex lanes. Derive model provenance from completed ledger entries instead.

  • src/workflows/dakar-review/main.ts#L676-L685: populate models from successful Flex ledger entries.
  • workflows/dakar-review.js#L1205-L1214: rebuild the artefact with accurate provenance.
🤖 Detailed instructions

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @src/workflows/dakar-review/main.ts around lines 676 - 685, The review record
currently derives models from legacy REVIEW_MODELS instead of execution results.
In src/workflows/dakar-review/main.ts lines 676-685, populate recordInput.models
from successful Flex ledger entries, preserving only models that actually
completed successfully; in workflows/dakar-review.js lines 1205-1214, apply the
same provenance-based model reconstruction when rebuilding the artefact.

@coderabbitai

coderabbitai Bot commented Jul 19, 2026

Copy link
Copy Markdown

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo '== Source symbols =='
ast-grep outline src/workflows/dakar-review/main.ts --items all --type function,const --match 'workflowMain|recordInput|ledger|REVIEW_MODELS'

echo '== Source provenance and ledger context =='
rg -n -C 12 'recordInput|REVIEW_MODELS|ledger\.push|LedgerEntry|attempts|completed|success' \
  src/workflows/dakar-review/main.ts

echo '== Generated artefact provenance context =='
rg -n -C 12 'recordInput|REVIEW_MODELS|ledger\.push|estimatedWorstCaseUsd|attempts' \
  workflows/dakar-review.js

echo '== Generation/freshness contract =='
rg -n -C 4 'workflow-freshness|build-workflow|dakar-review\.js' \
  AGENTS.md Makefile scripts/build-workflow.mjs

echo '== PR-local state =='
git status --short
git diff --check

Length of output: 1143


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo '== Source function map =='
ast-grep outline src/workflows/dakar-review/main.ts --items all --type function

echo '== Source provenance and ledger context =='
rg -n -C 14 'recordInput|REVIEW_MODELS|ledger\.push|estimatedWorstCaseUsd|attempts|models:' \
  src/workflows/dakar-review/main.ts

echo '== Generated artefact provenance context =='
rg -n -C 14 'recordInput|REVIEW_MODELS|ledger\.push|estimatedWorstCaseUsd|attempts|models:' \
  workflows/dakar-review.js

echo '== Generation/freshness contract =='
rg -n -C 4 'workflow-freshness|build-workflow|dakar-review\.js' \
  AGENTS.md Makefile scripts/build-workflow.mjs

echo '== PR-local state =='
git status --short
git diff --check

Length of output: 48777


@leynos Mark this finding resolved.

  • src/workflows/dakar-review/main.ts Lines 683-695 now derives recordedModels from the execution ledger rather than REVIEW_MODELS.
  • Successful finder calls contribute only when their task appears in taskResults; downgraded, aborted, and merely admitted finder calls are excluded.
  • The Terra audit model contributes only after its successful execution path reaches record construction.
  • Models are deduplicated while retaining first-execution order, then assigned through recordInput.models at Line 712.
  • workflows/dakar-review.js Lines 1233-1249 contains the matching generated implementation.
  • Retain REVIEW_MODELS for dry-run and legacy task-graph compatibility; it no longer supplies recorded review provenance.
  • git diff --check passed, and the working tree was clean.

@leynos

leynos commented Jul 19, 2026

Copy link
Copy Markdown
Owner Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 19, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@pandalump

Copy link
Copy Markdown
Collaborator

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 19, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@pandalump

Copy link
Copy Markdown
Collaborator

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 19, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai

coderabbitai Bot commented Jul 19, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

coderabbitai[bot]

This comment was marked as resolved.

Add the canonical docs/contents.md index and docs/repository-layout.md per
the documentation style guide, and land ADR 002 on the deterministic tiered
review-cost decision. Correct the ADR 002 title, which incorrectly duplicated
ADR 001's number.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
leynos added 3 commits July 24, 2026 09:23
Lead the migration guidance with the implemented pi adapters and name the
hook that injects the Flex service tier. Retain the Codex CLI limitation
and direct readers to the existing adapter-contract amendment.
Parse and validate the supported YAML subset before ODW starts, execute
normalized custom gates from trusted policy, and slice path guidance to
the finder packs whose evidence it governs.

Keep unsupported keys auditable but inert, preserve configuration
precedence and fail-closed accounting, and regenerate the workflow from
its authored TypeScript sources.
Distinguish the CLI operations surrounding `odw run` from model-free
report rendering inside the compiled workflow, and name the authored
function and SARIF renderer that own that path.
@leynos

leynos commented Jul 24, 2026

Copy link
Copy Markdown
Owner Author

@coderabbitai Have the following now been resolved?

tests/workflow-orchestration.test.mjs (1)

70-116: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Extract the shared compiled-workflow test harness instead of duplicating it. Both runWorkflow and renderOnce independently load workflows/dakar-review.js, patch out the meta export, build the same AsyncFunction body, and wire up nearly identical swallowFixtureFailure/parallel/pipeline mocks — a future change to the workflow's injected-primitive signature (this PR already added sleep) has to be applied twice by hand.

  • tests/workflow-orchestration.test.mjs#L70-L116: extract the compiled-source load, AsyncFunction construction, and mock-primitive wiring into a shared tests/helpers/run-workflow.mjs, parameterised for the extra retry-specific hooks (agentCalls, sleepDelays, nullParallelSlots, knobs).
  • tests/workflow-rendering.test.mjs#L35-L66: replace renderOnce's bespoke harness with a call into the same shared helper, omitting the retry-specific parameters it does not need.
🤖 Detailed instructions

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @tests/workflow-orchestration.test.mjs around lines 70 - 116, The compiled
workflow harness is duplicated across both test suites and must be centralized.
In tests/helpers/run-workflow.mjs, extract the shared workflow loading,
AsyncFunction construction, and parallel/pipeline failure mocks, parameterized
for agentCalls, sleepDelays, nullParallelSlots, and knobs; update runWorkflow in
tests/workflow-orchestration.test.mjs:70-116 to use it. Replace renderOnce in
tests/workflow-rendering.test.mjs:35-66 with the same helper, omitting
retry-specific parameters it does not need.

@coderabbitai

coderabbitai Bot commented Jul 24, 2026

Copy link
Copy Markdown

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo '== Relevant files =='
fd -a -t f . tests | rg '(^|/)(workflow-orchestration\.test\.mjs|workflow-rendering\.test\.mjs|run-workflow\.mjs)$' || true

echo '== Helper and test outlines =='
for file in \
  tests/helpers/run-workflow.mjs \
  tests/workflow-orchestration.test.mjs \
  tests/workflow-rendering.test.mjs
do
  if [ -f "$file" ]; then
    echo "-- $file"
    ast-grep outline "$file" --items all
  else
    echo "-- MISSING: $file"
  fi
done

echo '== Shared helper implementation and call sites =='
rg -n -C 8 \
  'export .*runWorkflow|function runWorkflow|AsyncFunction|readFileSync|replace\(|swallowFixtureFailure|parallel|pipeline|nullParallelSlots|agentCalls|sleepDelays|knobs' \
  tests/helpers/run-workflow.mjs \
  tests/workflow-orchestration.test.mjs \
  tests/workflow-rendering.test.mjs \
  2>/dev/null || true

echo '== Focused file contents =='
for file in \
  tests/helpers/run-workflow.mjs \
  tests/workflow-orchestration.test.mjs \
  tests/workflow-rendering.test.mjs
do
  if [ -f "$file" ]; then
    echo "-- $file"
    sed -n '1,190p' "$file"
  fi
done

echo '== Focused diff and integrity =='
git diff --check
git diff -- \
  tests/helpers/run-workflow.mjs \
  tests/workflow-orchestration.test.mjs \
  tests/workflow-rendering.test.mjs
git status --short

Length of output: 70079


@leynos Mark this finding resolved.

  • tests/helpers/run-workflow.mjs now centralizes generated-workflow loading, meta export rewriting, AsyncFunction construction, injected primitive wiring, fixture-failure handling, and the parallel/pipeline mocks.
  • runCompiledWorkflow() preserves the required ODW-like behaviour: listed nullParallelSlots resolve to null without invoking their thunk; intentional FixtureFailure instances resolve to null; unexpected errors propagate.
  • tests/workflow-orchestration.test.mjs now delegates to the shared helper and supplies agentCalls, sleepDelays, nullParallelSlots, and knobs through the helper’s workflow arguments.
  • tests/workflow-rendering.test.mjs now uses the same helper without retry-specific collections or controls.
  • The helper includes module and API documentation, and git diff --check passed.

Treat focused runtime-test execution as unverified here because this inspection environment does not execute repository code.

@leynos

leynos commented Jul 24, 2026

Copy link
Copy Markdown
Owner Author

@coderabbitai Have the following now been resolved?

dakar/bin/dakar-review.mjs
Lines 537 to 540 in 6322b23

 537   | const timeoutMs = (options.timeout || 900) * 1000 
 538   | const resultDeadline = Date.now() + timeoutMs 
 539   | process.stderr.write(`dakar-review: following ODW run ${runId}\n`) 
 540   | const logStatus = await followOdwLogs(odwBin, buildRunScopedArgs('logs', options, runId, ['--follow']), timeoutMs) 

Make the CLI timeout cover the default retry envelope

In the telemetry path inspected here, the default Flex configuration reports a 2,020-second worst-case review, but the CLI still stops following after 900 seconds. Under Flex backpressure where retries approach their configured timeouts, followOdwLogs returns 124 and the CLI exits without fetching or recording the eventual result; the default outer timeout should cover the retry envelope or incompatible settings should be rejected.

@coderabbitai

This comment was marked as resolved.

@leynos

leynos commented Jul 24, 2026

Copy link
Copy Markdown
Owner Author

@coderabbitai resume

@coderabbitai

coderabbitai Bot commented Jul 24, 2026

Copy link
Copy Markdown
✅ Action performed

Reviews resumed.

coderabbitai[bot]

This comment was marked as resolved.

Fail closed when trusted-base configuration cannot be read and normalise
Git revision paths across platforms. Include adapter overhead in the Terra
reservation and apply audit policy guidance against every candidate path,
while keeping the displayed path list bounded.

Update focused fixtures and the generated workflow artefact for the corrected
accounting contract.
coderabbitai[bot]

This comment was marked as resolved.

@pandalump

Copy link
Copy Markdown
Collaborator

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 24, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

coderabbitai[bot]

This comment was marked as resolved.

leynos added 3 commits July 24, 2026 23:07
Keep the production prompt surface aligned with the live Luna-to-Terra route
by removing the unused per-candidate verifier helper and its test-only
contract.
Describe the command root using the current checkout rather than committing a
user-specific absolute path.
Stream review stdout to its result file and scan usage telemetry while stderr
is teed, avoiding duplicate unbounded in-memory captures. Bound partial
telemetry lines and retain non-sensitive malformed diagnostics.

Show the full audit retry-chain reservation in the user-guide dry-run example.
@leynos

leynos commented Jul 24, 2026

Copy link
Copy Markdown
Owner Author

@coderabbitai Have the following failed checks now been resolved?

If further work is required, please provide an AI agent prompt for the remaining work to be done to address these failures.

Do not treat warnings as optional or aspirational. Where a change is out of scope for this PR, propose a GitHub issue unless one exists already. (Treat documentation and validation coverage as in scope).

❌ Failed checks (3 warnings, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 65.14% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
User-Facing Documentation ⚠️ Warning The user guide still omits reservedAuditChainUsd from the dry-run JSON example; only later prose mentions it, so the live contract isn’t clearly shown. Add reservedAuditChainUsd to the dry-run example and keep the example fields in sync with the live output contract.
Performance And Resource Use ⚠️ Warning scripts/live-review-harness.mjs buffers all stdout and rereads the full stderr log, so result/log size is unbounded and avoidably duplicated. Stream the usage scan from stderr instead of rereading the whole file, and write stdout to disk incrementally or cap the buffer with an explicit bound.

@leynos

leynos commented Jul 24, 2026

Copy link
Copy Markdown
Owner Author

@coderabbitai Have the following now been resolved?

src/workflows/dakar-review/prompts.ts (1)

1-22: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Delete the dead verificationPrompt helper. workflowMain uses auditPrompt for the live path, and verificationPrompt is only referenced by tests. Remove it or fold it into the test-only surface so it cannot drift from the prompt contract.

🤖 Detailed instructions

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @src/workflows/dakar-review/prompts.ts around lines 1 - 22, Remove the unused
verificationPrompt helper from the prompt module, keeping workflowMain’s
auditPrompt live path unchanged. Update or remove tests that reference
verificationPrompt so the production prompt surface contains only actively used
helpers and remains aligned with the prompt contract.

@coderabbitai

coderabbitai Bot commented Jul 24, 2026

Copy link
Copy Markdown

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo '== Current PR checks =='
gh pr checks 6 --repo leynos/dakar || true

echo '== Telemetry parser, aggregation, and stderr retention =='
ast-grep outline scripts/live-review-harness.mjs --items all --type function
rg -n -C 8 'extractUsageLines|sumUsage|summarize|priceReportedUsage|stderrChunks|stderr\.on|DAKAR-USAGE|malformedTelemetry' \
  scripts/live-review-harness.mjs tests/live-harness.test.mjs

echo '== Audit compaction implementation and property invariant =='
ast-grep outline src/workflows/dakar-review/candidates.ts --items all --type function
rg -n -C 7 'compactForAudit|auditCandidates\.length|min\(.*cap|deduplic|overCap|partition' \
  src/workflows/dakar-review/candidates.ts \
  tests/workflow-compact-audit.test.mjs \
  tests/workflow-properties.test.mjs

echo '== API-key probe and reported-usage documentation contracts =='
rg -n -i -C 5 'curl|OPENAI_API_KEY|authorization|header-file|header file|reportedUsage|inputTokens|cachedInputTokens|outputTokens|cacheRead|cacheWrite|ledgerTotalReportedUsd|reportedUsd' \
  docs/execplans/api-key-support.md docs/dakar-review-design.md

echo '== Docstring gate configuration and documentation on touched helpers =='
ast-grep outline scripts/check-docstrings.mjs --items all --type function
rg -n -C 5 'defaultSourcePatterns|threshold|80|coverage|undocumented|sourceFiles' \
  scripts/check-docstrings.mjs Makefile package.json
rg -n -C 2 '/\*\*|export (async )?function |^(async )?function ' \
  scripts/live-review-harness.mjs bin/dakar-review.mjs scripts/odw-config.mjs \
  adapters/pi/extensions/flex-tier.ts src/workflows/dakar-review

echo '== Existing observability issue =='
gh issue view 2 --repo leynos/dakar --json number,title,state,url

echo '== Relevant diff integrity =='
git diff --check
git status --short

Length of output: 215962


🏁 Script executed:

#!/bin/bash
set -euo pipefail

make docstrings

echo '== Verify reported-usage documentation references =='
rg -n -i -C 2 \
  'ledger.*reportedUsd|reportedUsd.*ledger|ledgerTotalReportedUsd|inputTokens|cachedInputTokens|outputTokens|reportedUsage|cacheRead|cacheWrite' \
  docs/dakar-review-design.md docs/execplans/api-key-support.md

Length of output: 682


@leynos: Keep Docstring Coverage unresolved.

make docstrings did not run: the inspection sandbox lacks make and exited with status 127. Because set -e stopped the script immediately, it also did not run the documentation-contract search. No current coverage numerator, denominator, percentage, or threshold result exists.

Run the following in CI or a development environment with the repository toolchain installed, then provide the output:

make docstrings

rg -n -i -C 2 \
  'ledger.*reportedUsd|reportedUsd.*ledger|ledgerTotalReportedUsd|inputTokens|cachedInputTokens|outputTokens|reportedUsage|cacheRead|cacheWrite' \
  docs/dakar-review-design.md docs/execplans/api-key-support.md

node --test tests/live-harness.test.mjs
node --test tests/workflow-compact-audit.test.mjs tests/workflow-properties.test.mjs
make workflow-freshness
make check
git diff --check

Correct the remaining docs/execplans/api-key-support.md historical claim that the ledger “gained reportedUsd” before rerunning the documentation search. Preserve inputTokens, cachedInputTokens, and maxOutputTokens only for the separate estimateWorstCaseUsd API contract.

Keep observability out of this PR; existing issue #2 remains the required follow-up.

@coderabbitai

This comment was marked as resolved.

@coderabbitai

coderabbitai Bot commented Jul 24, 2026

Copy link
Copy Markdown

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo '== Prompt module outline =='
ast-grep outline src/workflows/dakar-review/prompts.ts --items all

echo '== Prompt helper definitions and references =='
rg -n -C 5 '\bverificationPrompt\b|\bauditPrompt\b|\btaskPrompt\b' \
  src/workflows/dakar-review/prompts.ts \
  src/workflows/dakar-review/main.ts \
  tests \
  workflows/dakar-review.js || true

echo '== Focused prompt source =='
sed -n '1,260p' src/workflows/dakar-review/prompts.ts

echo '== Prompt-test imports and relevant assertions =='
sed -n '1,280p' tests/workflow-prompts.test.mjs

echo '== Focused changes and integrity =='
git diff --check
git diff -- src/workflows/dakar-review/prompts.ts tests/workflow-prompts.test.mjs workflows/dakar-review.js
git status --short

echo '== Generation contract =='
rg -n -C 3 'build-workflow|workflow-freshness|workflows/dakar-review\.js' \
  Makefile AGENTS.md scripts/build-workflow.mjs 2>/dev/null || true

Length of output: 41286


@leynos Mark this finding resolved.

  • src/workflows/dakar-review/prompts.ts no longer defines or exports verificationPrompt.
  • No references remain in production code, tests, or the generated workflows/dakar-review.js artefact.
  • src/workflows/dakar-review/main.ts retains the live route: taskPrompt for Luna finder packs and auditPrompt for the single Terra issue-set audit.
  • tests/workflow-prompts.test.mjs now tests only the active prompt surface: agentInstructionsBlock, taskPrompt, and auditPrompt.
  • git diff --check passed.

Treat focused test execution as unverified in this inspection environment, which cannot run repository code.

@leynos

leynos commented Jul 24, 2026

Copy link
Copy Markdown
Owner Author

@coderabbitai resume

@coderabbitai

coderabbitai Bot commented Jul 24, 2026

Copy link
Copy Markdown
✅ Action performed

Reviews resumed.

@leynos

leynos commented Jul 24, 2026

Copy link
Copy Markdown
Owner Author

@coderabbitai Have the following now been resolved?

src/workflows/dakar-review/main.ts (1)

195-201: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Include adapter overhead in the Terra reservation.
Lines 195-201 omit ADAPTER_OVERHEAD_TOKENS, although Luna adds it and the CLI contract states that overhead applies to every call. This underestimates Terra admission, retries, and ledger cost, allowing spend above the hard budget.

Proposed fix
-  inputTokens: TERRA_MAX_INPUT_TOKENS,
+  inputTokens: TERRA_MAX_INPUT_TOKENS + ADAPTER_OVERHEAD_TOKENS,
🤖 Detailed instructions

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @src/workflows/dakar-review/main.ts around lines 195 - 201, Update the TERRA
reservation calculation in RESERVED_AUDIT_USD to include ADAPTER_OVERHEAD_TOKENS
alongside the existing input token count, matching Luna’s reservation and the
CLI contract for every call. Preserve the existing Terra pricing and
output-token parameters.

src/workflows/dakar-review/prompts.ts (1)

127-152: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Apply path policy to every audit candidate.
Pass candidatePaths, not listedPaths, to policyGuidanceBlock(). The 40-path cap is only safe for display: audits can contain more candidate paths, and matching policy instructions for later candidates are currently omitted.

Proposed fix
-    policyGuidanceBlock(context.policy, listedPaths), '',
+    policyGuidanceBlock(context.policy, candidatePaths), '',

Add a regression case with more than 40 candidate paths and a policy instruction matching a later path.

🤖 Detailed instructions

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @src/workflows/dakar-review/prompts.ts around lines 127 - 152, Update the
audit prompt construction to pass the complete candidatePaths collection to
policyGuidanceBlock(), while continuing to use listedPaths only for the capped
display string. Add a regression case covering more than 40 candidate paths
where a policy instruction matches a path beyond the display cap, and verify
that instruction is included.

@coderabbitai

coderabbitai Bot commented Jul 24, 2026

Copy link
Copy Markdown

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo '== Target source outlines =='
ast-grep outline src/workflows/dakar-review/main.ts --items all --type function
ast-grep outline src/workflows/dakar-review/prompts.ts --items all --type function
ast-grep outline tests/workflow-prompts.test.mjs --items all --type function

echo '== Terra reservation accounting and related pricing/admission uses =='
rg -n -C 10 \
  'RESERVED_AUDIT_USD|TERRA_MAX_INPUT_TOKENS|ADAPTER_OVERHEAD_TOKENS|reservedAudit(Chain)?Usd|estimateWorstCaseUsd' \
  src/workflows/dakar-review/main.ts \
  tests/workflow-pricing.test.mjs \
  tests/workflow-dry-run.test.mjs \
  tests/workflow-orchestration.test.mjs \
  workflows/dakar-review.js

echo '== Audit-path display and policy-guidance wiring =='
rg -n -C 12 \
  'candidatePaths|listedPaths|policyGuidanceBlock|auditPrompt|slice\(0, 40\)|40-path|40 candidate|beyond.*40|beyond the.*cap' \
  src/workflows/dakar-review/prompts.ts \
  tests/workflow-prompts.test.mjs \
  workflows/dakar-review.js

echo '== Focused source/test excerpts =='
sed -n '175,220p' src/workflows/dakar-review/main.ts
sed -n '105,170p' src/workflows/dakar-review/prompts.ts
sed -n '1,300p' tests/workflow-prompts.test.mjs

echo '== Authored/generated and focused diff integrity =='
git diff --check
git diff -- \
  src/workflows/dakar-review/main.ts \
  src/workflows/dakar-review/prompts.ts \
  tests/workflow-prompts.test.mjs \
  workflows/dakar-review.js
git status --short

Length of output: 99498


@leynos Mark both findings resolved.

  1. Terra adapter overhead reservation

    • src/workflows/dakar-review/main.ts Line 198 now calculates the Terra reservation with:
      inputTokens: TERRA_MAX_INPUT_TOKENS + ADAPTER_OVERHEAD_TOKENS,
    • The resulting RESERVED_AUDIT_USD feeds the initial admission check, Terra audit admission, retry admission, and audit ledger entry.
    • workflows/dakar-review.js contains the matching generated expression.
    • tests/workflow-dry-run.test.mjs validates the corrected default reserve using (48,000 + 13,000) input tokens plus the configured output allowance.
  2. Audit policy guidance beyond the 40-path display cap

    • src/workflows/dakar-review/prompts.ts Line 120 now passes complete candidatePaths to policyGuidanceBlock().
    • listedPaths remains restricted to the capped changed-files display at Line 95.
    • tests/workflow-prompts.test.mjs includes the required regression: it creates 60 candidate paths, targets src/file-50.ts with a policy instruction, verifies that the path remains absent from the capped display, and verifies that its policy guidance is included in the audit prompt.

git diff --check passed. Treat runtime test execution as unverified in this inspection sandbox.

@leynos
leynos merged commit f51abbf into main Jul 24, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants