ee24b7786a
- generic SystemResourceProbe: owner-agnostic /proc/meminfo system RAM overlaid on the vendor GPU probe, wired on both managed and static paths so the VRAM/GPU/RAM gauge renders with an external llama-server (was dormant) - F-002: classify provider 4xx (e.g. llama.cpp 400) as terminal, not retryable (except 408/429); stops retrying deterministic request failures to exhaustion - F-003: FileSystemPromptLoader expands leading ~ / ~/ to user home - F-004: map InferenceFailedEvent + RetryAttemptedEvent to new ServerMessage.InferenceFailed / RetryAttempted, decoded+rendered in tui-go; ArtifactContentStoredEvent explicitly mapped to null (internal, no warn) - tests: tilde expansion, SystemResourceProbe (static/managed/fail-soft)
481 lines
35 KiB
Markdown
481 lines
35 KiB
Markdown
# correx QA / Audit Report — 2026-06-08
|
||
|
||
Live operator session (Phase 3). Provider: local llama-server via env-var fallback
|
||
(`llama-cpp:default` @ 127.0.0.1:10000). Target: `qa/sample-repo/` (tag `clean`).
|
||
|
||
## Run context / repro prompt
|
||
|
||
Workflow: `role_pipeline` (analyst→architect→planner→implementer⇄reviewer).
|
||
Target workspace (TUI `Hello` workingDir): `/home/kami/Programs/correx/qa/sample-repo`
|
||
(reset between runs with `git -C qa/sample-repo reset --hard clean`).
|
||
Provider: env-var fallback `llama-cpp:default` @ 127.0.0.1:10000 (start llama-server first).
|
||
|
||
Operator prompt used for every run so far:
|
||
|
||
```
|
||
Fix the failing test in this repository. Running `python3 -m pytest` shows
|
||
tests/test_calc.py::test_average_empty failing: calc.average([]) raises
|
||
ZeroDivisionError, but it should return 0.0 for an empty list. Diagnose the
|
||
cause in src/sampleapp/calc.py, implement the fix, and verify all tests pass.
|
||
```
|
||
|
||
Patches applied this session (rebuild + restart server to load):
|
||
- F-001: `LlamaCppInferenceProvider.kt` — drop grammar when tools present.
|
||
- F-005: `FileReadConfig`/`Main` workspace builder/`FileReadTool` — workingDir-relative
|
||
path resolution.
|
||
- F-007: validator content-map (drops CAS dep) + `ArtifactContentStoredEvent` slot→hash
|
||
recording. No server restart needed for validation correctness.
|
||
- F-008: `TomlWorkflowLoader` — propagate stage `token_budget` → `generationConfig.maxTokens`.
|
||
- F-009: `FileReadTool` file-not-found → `recoverable=true`; `SessionOrchestrator` feeds
|
||
recoverable tool errors back into the loop instead of failing the stage.
|
||
- F-010: `SessionOrchestrator` — reject premature `stage_complete` when an LLM-emitted slot
|
||
is unfilled; nudge the model to emit the artifact.
|
||
- F-011: artifact schemas (`analysis`/`design`/`impl_plan`) — list fields `string` → `array`.
|
||
Live config under `~/.config/correx/schemas/` + repo `docs/schemas/` synced.
|
||
- F-012: `SessionOrchestrator.stripCodeFence` — strip outer ```` ``` ```` fence from
|
||
LLM-emitted artifacts before hash/store/validate.
|
||
- F-013: `ValidationPipeline` — payload-validation failures retryable, structural ones not.
|
||
- F-014 + diff: `FileEditTool` — schema matches real params (`target`/`replacement`/`patch`);
|
||
successful edits emit a unified diff.
|
||
- F-015: `SessionOrchestrator` — record tool-execution events + materialise a real
|
||
`file_written` artifact from the on-disk write; gate validation so a no-write stage cannot
|
||
be rubber-stamped.
|
||
- F-016: `Main.kt` — `stageTimeoutMs` 60s → 180s (still a hardcode; should be config-wired).
|
||
- F-018: `SessionOrchestrator` — write-nudge: when a stage owes a `file_written` artifact but
|
||
the model ended a turn with content (or called `stage_complete`) without writing, push back a
|
||
corrective tool-result instructing it to invoke `file_edit`/`file_write` (bounded by
|
||
`MAX_TOOL_ROUNDS`). Extends the F-010 premature-completion guard to file-write stages.
|
||
Regression fix: `FileReadToolTest` updated for the F-009 `recoverable=true` change.
|
||
- F-019: `FileWrittenArtifact`/`FileWrittenKind` gain a `diff` field; `file_edit`+`file_write`
|
||
emit a unified diff (shared `DiffUtil.unifiedDiff`); `materializeFileWritten` threads it into the
|
||
artifact the reviewer reads, so the reviewer can verify the change.
|
||
- F-020: `FileEditTool` — model-correctable failures (bad patch, target-not-found, bad params,
|
||
unknown op) now `recoverable=true` (feed back, don't abort); tool/`operation` descriptions +
|
||
implementer prompt steer toward `replace`/`file_write` over the fragile `patch` op.
|
||
- F-021: FIXED — resume `rehydrate()` now consumes `ArtifactContentStoredEvent` (slot→hash)
|
||
and fetches CAS bytes by content hash instead of passing the slot name to the hash-keyed store.
|
||
|
||
**Key reproducibility note:** the live config the server reads is `~/.config/correx/`
|
||
(`configDir = ConfigLoader.configPath().parent`), NOT the repo. Code fixes load via repo
|
||
build jars on the classpath; all config/prompts/schemas come from `~/.config/correx/`. The
|
||
repo `docs/schemas/` were stale relative to live — a clean-checkout reproduction will differ.
|
||
|
||
## Findings
|
||
|
||
### F-001 — grammar + tools sent together → llama.cpp 400, every tool+artifact stage dies
|
||
- **invariant/contract:** functional (workflows run end-to-end) / #7 path (artifact constraint)
|
||
- **subsystem:** infrastructure/inference (llama_cpp)
|
||
- **severity:** blocker
|
||
- **evidence:** `logs/current.log` analyst stage: `llama-server returned 400 Bad Request:
|
||
"Cannot use custom grammar constraints with tools."`. Request body carries both
|
||
`"grammar": "root ::= ..."` and `"tools":[file_read, stage_complete]`.
|
||
- **repro:** `correx run role_pipeline` (any prompt) against sample-repo. Analyst declares
|
||
`allowed_tools=[file_read, ShellTool]` AND `produces=[analysis]`. Fails immediately.
|
||
- **root cause:** `LlamaCppInferenceProvider.kt:103-104` sets `grammar` and `tools`
|
||
unconditionally. llama.cpp's server forbids the combination. ANY stage that both has
|
||
tools and a JSON `responseFormat` is unrunnable on llama.cpp. role_pipeline analyst +
|
||
implementer both do → pipeline cannot start.
|
||
- **fix direction:** when tools are present, drop the GBNF grammar and rely on
|
||
post-hoc schema validation + retry (invariant #7 still holds — output is validated);
|
||
OR gate the artifact emission into a separate tool-free final inference turn.
|
||
- **status:** PATCHED (workaround) — `LlamaCppInferenceProvider.kt`: grammar suppressed
|
||
when tools are present; artifact now constrained by validation+retry only. Requires
|
||
server restart to take effect. Proper fix tracked below.
|
||
- **follow-up TODO (proper fix):** add a first-class artifact-emission tool — the LLM
|
||
calls `emit_artifact(path, content, description)` like it calls file_read/file_write,
|
||
making artifact production an explicit schema-checked tool call instead of a grammar
|
||
constraint that can't coexist with tools. TODO left in code at the patch site.
|
||
|
||
### F-002 — non-retryable 400 is retried to exhaustion
|
||
- **invariant/contract:** functional (retry accounting)
|
||
- **subsystem:** core/kernel (orchestration retry policy)
|
||
- **severity:** minor
|
||
- **evidence:** orchestrator marks the 400 `retryable=true`; retries 1/3, 2/3, 3/3 all
|
||
identical 400s within ~80ms, then `WorkflowFailedEvent retryExhausted=true`.
|
||
- **suspected cause:** retry classifier treats all provider failures as retryable. A 400
|
||
`invalid_request_error` is deterministic request-construction failure → should be terminal.
|
||
- **status:** FIXED (2026-06-10) — `SessionOrchestrator.isRetryableFailure` parses the
|
||
provider status from the failure reason; 4xx is terminal (except 408/429), 5xx/network/
|
||
unknown stay retryable. So a 400 now fails fast instead of burning the retry budget.
|
||
|
||
### F-003 — `~` not expanded in default_system prompt path
|
||
- **subsystem:** core/kernel (prompt loading) / core/config
|
||
- **severity:** minor
|
||
- **evidence:** `failed to load prompt '~/.config/correx/prompts/default_system.md':
|
||
Prompt not found (searched: .../prompts/~/.config/correx/prompts/default_system.md)` —
|
||
tilde concatenated literally onto the config dir. Logged ERROR on every attempt.
|
||
Non-fatal (run proceeds) but noisy + the default system prompt is silently absent.
|
||
- **status:** FIXED (2026-06-10) — `FileSystemPromptLoader.expandTilde` resolves a leading
|
||
`~`/`~/` to the user home before lookup (unit-tested).
|
||
|
||
### F-005 — file_read jails relative paths against process cwd, not the workspace
|
||
- **invariant/contract:** functional (tool jailing) / two-plane consistency (#9)
|
||
- **subsystem:** infrastructure/tools/filesystem (FileReadTool) + apps/server wiring
|
||
- **severity:** major (blocks any file_read of a relative path unless server launched
|
||
from inside the workspace)
|
||
- **evidence:** `logs/current.log` session c0eab7ba: analyst emitted
|
||
`file_read(path=tests/test_calc.py)`. Plane-2 PATH_CONTAINMENT PROCEEDed
|
||
(`resolved=.../qa/sample-repo/tests/test_calc.py, inWorkspace=true`), but the tool's own
|
||
`validateRequest` rejected it: `Path 'tests/test_calc.py' is not in the allowed list`
|
||
→ stage produced no artifact → surfaced as the misleading `no transition condition
|
||
matched from stage analyst`.
|
||
- **root cause:** `FileReadTool` resolved the relative path via `Paths.get(p).toAbsolutePath()`
|
||
= JVM process cwd (the correx repo root, where the server was launched), not the bound
|
||
workspace. `FileReadConfig` had no `workingDir` field at all, unlike FileWrite/FileEdit
|
||
which carry `workingDir` and call `resolvePath()`. So the two file-access planes anchored
|
||
relative paths to different roots.
|
||
- **status:** PATCHED — added `workingDir` to `FileReadConfig`, wired
|
||
`workspace.workingDir` in Main's per-workspace tool builder, and gave `FileReadTool` a
|
||
`resolvePath()` mirroring FileWriteTool. Requires server restart.
|
||
- **note:** the operator-facing failure reason (`no transition condition matched`) masked
|
||
the real cause (tool denial). See F-004 family — failure-reason surfacing is lossy.
|
||
|
||
### F-007 — ArtifactPayloadValidator conflates slot name with CAS content-hash → crash
|
||
- **invariant/contract:** functional (artifacts validate) / #7 (LLM output untrusted
|
||
until validated — the validation step itself is broken)
|
||
- **subsystem:** core/validation + core/artifacts-store contract vs infrastructure CAS
|
||
- **severity:** BLOCKER (every workflow with a typed `produces` slot crashes at the first
|
||
post-stage validation against the real CAS store)
|
||
- **evidence:** `logs/current.log` session 28c818c1: analyst COMPLETED
|
||
(responseArtifactId=82da21d5…, a real blake3 hash), then
|
||
`runSession ... failed: hex string must have even length` —
|
||
`CasArtifactStore.hexToBytes` at `CasArtifactStore.kt:102`, called from
|
||
`ArtifactPayloadValidator.validateSlot:36`.
|
||
- **root cause:** `ArtifactPayloadValidator.validateSlot` calls
|
||
`artifactStore.get(slot.name)`. `slot.name` is the LOGICAL slot id ("analysis"), but
|
||
`CasArtifactStore.get` requires `ArtifactId.value` to be a hex content-hash
|
||
(`id.value.hexToBytes()`). "analysis" → odd-length → `require` throws. The validator has
|
||
no slot-name→content-hash mapping: `ValidationContext` carries only the static graph +
|
||
`SessionState`, and `SessionState` holds NO produced-artifact map. So the validator
|
||
cannot locate the bytes it is supposed to validate.
|
||
- **deeper implication:** this path has apparently never been exercised end-to-end against
|
||
the real content-addressed CAS — it only "works" if the ArtifactStore is keyed by literal
|
||
slot name (an in-memory test double). The `ArtifactStore.get(ArtifactId)` contract is
|
||
ambiguous: the validator treats ArtifactId as a logical name, the CAS treats it as a hash.
|
||
- **fix direction (NOT a one-liner):** the run's produced-artifact mapping
|
||
(slot name → ArtifactId(hash)) must be derived from the event log (an ArtifactProduced-type
|
||
event projection per invariant #1) and threaded into `ValidationContext`, so the validator
|
||
resolves slot.name → stored hash before `get`. Cross-module change
|
||
(events → projection → context builder → validator). Audit-only: NOT patched.
|
||
- **status:** FIXED.
|
||
- **fix applied:** two changes (validation crash + durable mapping).
|
||
1. *Validator no longer touches the CAS.* `ValidationContext` gained
|
||
`producedArtifactContent: Map<String,String>` (slot.name.value → payload). The
|
||
orchestrator populates it from the in-memory `artifactContentCache` for every produced
|
||
slot across `graph.stages` (scoped to the session). `ArtifactPayloadValidator` reads
|
||
from that map and **dropped its `ArtifactStore` dependency entirely** — the slot-name
|
||
vs content-hash conflation is gone. `Main.kt` wiring updated. Validation stays live-only
|
||
(never runs during replay) so invariant #8 is unaffected.
|
||
2. *Slot→CAS-hash mapping now recorded as an event* (invariant #9 — env observation
|
||
recorded at the moment it runs). New `ArtifactContentStoredEvent(artifactId=logical
|
||
slot, contentHash=CAS blake3 hash, sessionId, stageId)`, registered in
|
||
`Serialization.kt`, emitted at both CAS write sites (LLM-emitted slot path + tool/
|
||
process_result path; the tool path previously discarded the `put()` return value). This
|
||
is the durable record that lets content be recovered from CAS by hash after a cold start.
|
||
- **remaining follow-up (not in this fix):** the validator/`needs`-rehydration still read the
|
||
*in-memory* `artifactContentCache`, which is lost on restart (the F-001 rehydration-gap
|
||
suspect). Closing that means resolving slot→hash→CAS from the new
|
||
`ArtifactContentStoredEvent` on cold start / re-validation. Foundation is now in place.
|
||
|
||
### F-006 — TUI input pane render glitch
|
||
- **subsystem:** apps/tui-go
|
||
- **severity:** minor (cosmetic)
|
||
- **evidence:** operator screenshot 16:07 — a garbled/smeared rectangular artifact in the
|
||
bottom-right of the input pane (uncleared region / stale cell buffer) while the prompt
|
||
text rendered normally on the left.
|
||
- **repro:** operator confirms it appears **only on multi-line paste** into the input box;
|
||
single-line / typed input renders clean. Likely the input widget doesn't repaint the full
|
||
region when a pasted block changes line count.
|
||
- **status:** confirmed, repro narrowed (multi-line paste only)
|
||
|
||
### F-004 — InferenceFailedEvent / RetryAttemptedEvent unmapped to TUI bridge
|
||
- **invariant/contract:** functional (operator observability)
|
||
- **subsystem:** apps/server (DomainEventMapper)
|
||
- **severity:** minor
|
||
- **evidence:** `DomainEventMapper: unmapped payload type=InferenceFailedEvent` and
|
||
`=RetryAttemptedEvent` repeated. Operator sees failure only via router narration, not a
|
||
structured failure/retry surface in the TUI.
|
||
- **status:** FIXED (2026-06-10) — new `ServerMessage.InferenceFailed` /
|
||
`RetryAttempted` variants; `DomainEventMapper` arms map `InferenceFailedEvent` /
|
||
`RetryAttemptedEvent`; tui-go decodes + renders them as session events. The internal
|
||
`ArtifactContentStoredEvent` is now explicitly mapped to null (no operator surface, no warn).
|
||
|
||
### F-008 — stage `token_budget` not propagated to provider `max_tokens` → truncation
|
||
- **invariant/contract:** functional (workflows run end-to-end) / #7 (validatable output)
|
||
- **subsystem:** infrastructure/workflow (TomlWorkflowLoader) + core/transitions (StageConfig)
|
||
- **severity:** major
|
||
- **evidence:** session `f5046936`: `role_pipeline.toml` analyst declares `token_budget=16384`,
|
||
but every request carried `"max_tokens":2048` (the StageConfig default). The analyst's 2nd
|
||
inference hit `completionTokens=2048 finishReason=length` → JSON truncated mid-string →
|
||
artifact validation failed → `no transition condition matched`.
|
||
- **root cause:** `TomlWorkflowLoader.toWorkflowGraph` set `StageConfig.tokenBudget` from the
|
||
TOML but never populated `generationConfig`, so the 2048 default `maxTokens` reached the
|
||
provider. `token_budget` fed context budgeting only, never the completion cap.
|
||
- **status:** FIXED — loader sets `generationConfig.maxTokens = s.tokenBudget`. Confirmed live
|
||
(session `1876d22a`: 337 completion tokens, no truncation).
|
||
|
||
### F-009 — tool-execution errors abort the stage instead of feeding back to the model
|
||
- **invariant/contract:** functional / #3 (tools suggest; core records and continues)
|
||
- **subsystem:** infrastructure/tools/filesystem (FileReadTool) + core/kernel (orchestrator loop)
|
||
- **severity:** major
|
||
- **evidence:** session `1876d22a`: analyst speculatively read `tests/__init__.py` (doesn't
|
||
exist); `FileReadTool` returned `recoverable=false` → orchestrator treated it as FATAL →
|
||
`stage failed reason=File not found: tests/__init__.py retryable=false`. Both FATAL and
|
||
recoverable ERROR tool results aborted the stage (the `ERROR:` feedback entry was built then
|
||
discarded) — the model never got to adapt.
|
||
- **root cause:** `FileReadTool` marked missing-file `recoverable=false`; `SessionOrchestrator`
|
||
loop returned `StageExecutionResult.Failure` for any ERROR/FATAL tool entry instead of
|
||
appending recoverable errors to context and continuing.
|
||
- **status:** FIXED — missing file → `recoverable=true`; recoverable errors flow back into the
|
||
tool loop (bounded by `MAX_TOOL_ROUNDS`); only FATAL aborts. Confirmed live.
|
||
|
||
### F-010 — premature `stage_complete` stores an empty artifact → validation fails
|
||
- **invariant/contract:** functional / #7 (the artifact-emission contract)
|
||
- **subsystem:** core/kernel (SessionOrchestrator)
|
||
- **severity:** major (direct consequence of the F-001 grammar drop)
|
||
- **evidence:** session `ea968b72`: after reading files the analyst called `stage_complete`
|
||
with empty content, never emitting the `analysis` JSON. The orchestrator captured
|
||
`response.text` (empty, hash `af1349b9…`) as the artifact → `validation failed`.
|
||
- **root cause:** once the GBNF grammar is dropped for tool-bearing stages (F-001), nothing
|
||
forces artifact emission; the model can end the stage via `stage_complete` without producing
|
||
its declared slot.
|
||
- **status:** FIXED — `stage_complete` is rejected while an LLM-emitted slot is unfilled; a
|
||
corrective tool-result nudges the model to emit the artifact (bounded by `MAX_TOOL_ROUNDS`).
|
||
Confirmed live (session `c02cf17e`: nudge worked, real artifact emitted on the 3rd turn).
|
||
- **follow-up (proper fix):** the first-class `emit_artifact` tool from the F-001 TODO would
|
||
make artifact production an explicit schema-checked tool call.
|
||
|
||
### F-011 — artifact schemas over-constrain list fields as `string` → validation rejects arrays
|
||
- **invariant/contract:** functional (artifacts validate)
|
||
- **subsystem:** config schemas (`~/.config/correx/schemas/`) + core/validation
|
||
- **severity:** major (latent — masked by F-007/F-008/F-010 until artifacts validated cleanly)
|
||
- **evidence:** session `c02cf17e`: analyst emitted a complete `analysis` with `requirements`
|
||
as a JSON **array**; schema declared `requirements`/`affected_areas` as `"type":"string"`
|
||
(newline-separated). `JsonSchemaValidator` (single-type, no unions) rejected the array.
|
||
The model emits arrays every run.
|
||
- **root cause:** schema design mismatch with natural model output; the minimal validator
|
||
cannot express a string|array union.
|
||
- **status:** FIXED — `requirements`/`affected_areas` (analysis), `components`/`risks` (design),
|
||
`steps`/`verification` (impl_plan) changed `string`→`array`. Live config + repo docs synced.
|
||
`review_report.verdict` left `string` (the `verdict == approved` transition depends on it).
|
||
- **note:** validation strictness itself is correct (#7 held — it rejected mismatched output).
|
||
|
||
### F-012 — LLM wraps artifact JSON in a markdown code fence → unparseable
|
||
- **invariant/contract:** functional (artifacts validate)
|
||
- **subsystem:** core/kernel (SessionOrchestrator)
|
||
- **severity:** major (model-nondeterministic — any stage, any run)
|
||
- **evidence:** session `231179c5`: planner emitted ```` ```json … ``` ```` for `impl_plan`;
|
||
the validator parses raw JSON → parse failure → `validation failed`. analyst/architect that
|
||
run emitted bare JSON and passed — the difference is pure model nondeterminism.
|
||
- **status:** FIXED — `stripCodeFence` in `runInference` removes an outer fence wrapper before
|
||
the artifact is hashed/stored/validated (CAS, cache, validation stay consistent). Conservative:
|
||
only fires when the whole response is a fenced block.
|
||
|
||
### F-013 — validation failures never retried → one bad LLM output kills the workflow
|
||
- **invariant/contract:** #7 ("Rejected + retried (max_retries)") — the plan's stated contract
|
||
- **subsystem:** core/validation (ValidationPipeline)
|
||
- **severity:** BLOCKER (robustness — the dominant failure mode with a nondeterministic model)
|
||
- **evidence:** session `67776482`: analyst emitted a malformed artifact (a tool-call shape as
|
||
JSON content); validation rejected it `retryable=false` and the workflow died, despite
|
||
`max_retries=2`. The previous run's analyst was flawless — pure nondeterminism with zero
|
||
recovery.
|
||
- **root cause:** `ValidationPipeline` marked **all** ERROR-severity validation outcomes
|
||
`retryable=false` ("structural errors must be fixed"), conflating graph misconfiguration with
|
||
transient bad LLM output.
|
||
- **status:** FIXED — payload-validation failures (`artifact_payload` section) are now retryable
|
||
(stage re-runs up to `max_retries`); structural/graph errors stay terminal. This wired the
|
||
invariant-#7 contract that was never actually implemented. Stabilised the pipeline end-to-end
|
||
(session `c6212fd0` completed 9 stages after this).
|
||
|
||
### F-014 — `file_edit` schema misdescribes its own parameters → edits never apply
|
||
- **invariant/contract:** functional (tool side effects) / #5
|
||
- **subsystem:** infrastructure/tools/filesystem (FileEditTool)
|
||
- **severity:** BLOCKER
|
||
- **evidence:** session `c6212fd0`: implementer called `file_edit operation=patch` with
|
||
`old_str`/`new_str` (exactly what the schema advertised) → `Missing 'patch' parameter for
|
||
'patch' operation`. Across all 4 implementer rounds, not one edit applied; the file stayed
|
||
unchanged.
|
||
- **root cause:** schema documented `content`/`old_str`/`new_str`; the code reads `content`
|
||
(append), `target`/`replacement` (replace), `patch` (unified diff, via `patch -p1`).
|
||
`old_str`/`new_str` are read by nothing; `target`/`replacement`/`patch` are documented
|
||
nowhere. A schema-following model can never `replace` or `patch`.
|
||
- **status:** FIXED — schema now documents the real params (`target`/`replacement`/`patch`),
|
||
drops the dead `old_str`/`new_str`. Plus: successful edits emit a `diff -u` unified diff
|
||
(operator-requested), surfaced in `ToolReceipt.diff` and the `file_written` artifact.
|
||
|
||
### F-015 — tool-output artifacts auto-validated without verifying the side effect → false success
|
||
- **invariant/contract:** #5 (all side effects captured in events) / functional (real output)
|
||
- **subsystem:** core/kernel (SessionOrchestrator emitToolArtifacts + dispatch)
|
||
- **severity:** BLOCKER (a green `done` run that changed nothing — worst-case for a kernel)
|
||
- **evidence:** session `c6212fd0`: workflow completed `terminalStage=done, verdict=approved,
|
||
9 stages`, but `git status` clean and `calc.average` bug intact. `emitToolArtifacts`
|
||
unconditionally emitted `ArtifactCreated→Validating→Validated` for the `patch` slot every
|
||
implementer round regardless of any write; `validateSlotAsFileWritten` early-returns when the
|
||
slot has no content (it never had any) → rubber-stamped. The reviewer then approved a no-op.
|
||
- **root cause:** file-mutating tools never produced a `file_written` artifact (unlike shell
|
||
tools → `ProcessResultArtifact`); tool executions emitted no completion/file-write events at
|
||
all, so nothing tied the `patch` slot to a real write.
|
||
- **status:** FIXED — dispatch now emits `ToolExecutionCompleted`/`Failed` + `FileWrittenEvent`,
|
||
and **materialises a real `file_written` artifact** (path, content hash, size, diff) from the
|
||
on-disk result. `emitToolArtifacts` gates the `file_written` slot on that content: no write →
|
||
no artifact → `verifyProduces` fails the stage → retry → honest failure (confirmed live,
|
||
session `57d5795d`: `did not produce declared artifacts: patch retryExhausted=true`).
|
||
|
||
### F-016 — `stageTimeoutMs` hardcoded at 60s, too tight for local models + not configurable
|
||
- **invariant/contract:** functional (operability)
|
||
- **subsystem:** core/kernel (OrchestrationConfig) + apps/server (Main)
|
||
- **severity:** major
|
||
- **evidence:** session `57d5795d`: ~15 `TIMEOUT: Timed out waiting for 60000 ms` across the
|
||
run (7–8 on implementer alone). Large implementer/reviewer turns (plan + design + file
|
||
contents) were cut off mid-generation, burning all retries → `did not produce patch`.
|
||
- **root cause:** `OrchestrationConfig.stageTimeoutMs` defaults to `60_000L` and is hardcoded
|
||
in `Main` (`defaultOrchestrationConfig`) with no config wiring.
|
||
- **status:** FIXED — now a real config knob `orchestration.stage_timeout_ms`
|
||
(`CorrexConfig.OrchestrationKnobs`, default 180s), parsed by `ConfigLoader`, written by
|
||
`CorrexConfigWriter`, live-editable via `EditableConfig`, wired in `Main.kt`.
|
||
|
||
### F-017 (observation, not a correx defect) — local model does not reliably issue write tools
|
||
- **invariant/contract:** n/a (model capability)
|
||
- **subsystem:** n/a (provider model `llama-cpp:default`)
|
||
- **severity:** informational
|
||
- **evidence:** session `57d5795d` implementer: 6 inference attempts, **only `file_read` calls**
|
||
— zero `file_edit`/`file_write`, zero `stage_complete`. Even on turns that completed within
|
||
timeout, the model read repeatedly and never attempted the edit it was instructed to make.
|
||
- **note:** correx offers `file_write`/`file_edit` correctly and the implementer prompt
|
||
explicitly instructs tool-based writing. This is a model-selection conclusion: role_pipeline's
|
||
correctness is sound, but this local model is too weak for the implementer stage. Re-test the
|
||
implementer with a stronger model before drawing functional conclusions about that stage.
|
||
|
||
---
|
||
|
||
### F-018 — implementer emits the change as prose instead of invoking the write tool
|
||
- **invariant/contract:** functional (workflows write expected files) — mitigation for F-017
|
||
- **subsystem:** core/kernel (SessionOrchestrator tool loop)
|
||
- **severity:** major (robustness; the stage silently failed instead of coaxing a write)
|
||
- **evidence:** sessions `57d5795d`, `fef41879`: implementer dispatched only `file_read`
|
||
(2–3 calls), then ended turns with `finishReason=stop`/`length` — emitting the corrected code
|
||
as message content, never calling `file_edit`/`file_write`. The write tools WERE offered in the
|
||
request (confirmed: `file_edit`/`file_write` present in the implementer's tools array). The
|
||
stage then failed `did not produce declared artifacts: patch` with no attempt to redirect the
|
||
model.
|
||
- **status:** FIXED (staged) — the tool loop now nudges: a content turn (or a `stage_complete`)
|
||
while a `file_written` slot is unfilled triggers a corrective tool-result ("you MUST call
|
||
file_edit/file_write …") and a re-run, bounded by `MAX_TOOL_ROUNDS`. If the model never
|
||
complies it fails honestly at the round cap — a clean signal that the model (not correx) is the
|
||
wall. Unit tests green; awaiting a live run to confirm it tips this model into tool-calling.
|
||
|
||
### F-019 — `file_written` artifact carries only metadata → reviewer can't see the change, loops forever
|
||
- **invariant/contract:** functional (refinement loop / reviewer can verify work)
|
||
- **subsystem:** core/artifacts (FileWrittenArtifact) + core/kernel (needs-injection) + the diff path
|
||
- **severity:** major
|
||
- **evidence:** session `265f5fe6` — the implementer **wrote correctly** (`file_write`, real diff;
|
||
`calc.average` fixed, all 7 pytest pass). But the reviewer (`needs=[patch, impl_plan]`) saw only
|
||
the `FileWrittenArtifact` metadata and returned `changes_requested`: *"Cannot verify the patch
|
||
content — only metadata (hash, size, mode) was provided, not the actual code changes."* The
|
||
refinement loop hit its 2-iteration cap → `WorkflowFailed: refinement loop exceeded 2 iterations`.
|
||
- **root cause:** `FileWrittenArtifact` is `(path, contentHash, size, mode)` — no diff, no content.
|
||
The diff added in F-014 lives on `ToolReceipt.diff` / the tool output, but is NOT threaded into
|
||
the artifact the reviewer's `needs` injection reads. (Also: the implementer used `file_write`,
|
||
which produces no diff at all.) So the reviewer is structurally unable to review the change — the
|
||
flip side of the operator's original "patch tool should provide a diff" point.
|
||
- **status:** FIXED (staged) — `FileWrittenArtifact` gained a `diff` field; both `file_edit` and
|
||
`file_write` now compute a `diff -u` unified diff (shared `unifiedDiff` helper in
|
||
`DiffUtil.kt`) and surface it via `ToolResult.metadata["diff"]`; `materializeFileWritten`
|
||
threads it into the artifact the reviewer's `needs`-injection reads. Unit tests green (artifacts
|
||
6, filesystem 38, kernel, validation). Awaiting a live run to confirm the reviewer can now verify
|
||
the change and the loop converges.
|
||
- **secondary observation:** reviewer verdicts were nondeterministic — one turn emitted
|
||
`verdict=approved` ("approved at tier T3…"), later turns `changes_requested`. With no diff to
|
||
anchor on, the verdict is essentially ungrounded.
|
||
|
||
### F-020 — `file_edit` failures are fatal + model defaults to the fragile `patch` op
|
||
- **invariant/contract:** functional (tool robustness) / #3 (tools suggest, core records & continues)
|
||
- **subsystem:** infrastructure/tools/filesystem (FileEditTool) + implementer prompt
|
||
- **severity:** major
|
||
- **evidence:** session `6ebe0c50` — the implementer correctly called `file_edit` (F-014 works)
|
||
but chose `operation=patch` and produced a non-applying unified diff → `patch -p1` exit 2 →
|
||
`Patch failed with exit code 2`. The failure was `recoverable=false` (FATAL) → the stage aborted
|
||
with no chance for the model to fall back to `replace`/`file_write`. File unchanged.
|
||
(Also observed: a client-side `Request timeout has expired` independent of `stageTimeoutMs`.)
|
||
- **root cause:** (a) model-correctable `file_edit` failures (bad patch, target-not-found, bad
|
||
params, unknown op) were all marked `recoverable=false`, so they aborted instead of feeding the
|
||
error back (same class as F-009); (b) nothing steered the finicky local model away from the
|
||
fragile `patch` op toward the reliable `replace`/`file_write`.
|
||
- **status:** FIXED (staged) — (a) those failures are now `recoverable=true` → fed back into the
|
||
loop so the model can self-correct or fall back (and F-018's write-nudge re-engages); the patch
|
||
failure message now explicitly suggests `replace`/file_write. (b) Tool description + `operation`
|
||
description recommend `replace`/file_write over `patch`; implementer prompt rewritten to call a
|
||
write tool (prefer file_write/replace, avoid patch) and not `stage_complete` before writing.
|
||
Filesystem tests green (38). Awaiting a live run.
|
||
- **note (model-swap groundwork):** correx already has per-stage model selection in the data model
|
||
(`StageConfig.modelId`, capability routing, multi-provider config) but it is **unwired** — the
|
||
TOML loader parses neither a per-stage `model` nor `capabilities`, and `DefaultInferenceRouter`'s
|
||
`modelId` overload falls back to capability routing. Enabling the llm_pipeline-style per-stage
|
||
model swap (e.g. a tool-strong model for implementer/reviewer) needs only targeted wiring, not
|
||
new architecture. Deferred pending the robustness-first decision.
|
||
|
||
### F-021 — resume-after-restart `rehydrate()` reuses the F-007 slot-name-vs-CAS-hash conflation
|
||
- **invariant/contract:** #1 (event log is the only source of truth) / #8 (replay/restore is
|
||
environment-independent) — resume-after-restart
|
||
- **subsystem:** core/kernel (DefaultSessionOrchestrator.rehydrate)
|
||
- **severity:** major
|
||
- **evidence (static, not yet probed live):** `DefaultSessionOrchestrator.rehydrate` (line 171)
|
||
walks `ArtifactValidatedEvent`s and calls `artifactStore.get(p.artifactId)` to repopulate
|
||
`artifactContentCache`. `p.artifactId` is the **logical slot id** ("analysis", "patch"), but
|
||
`CasArtifactStore.get` requires a content **hash** (`id.value.hexToBytes()`). `get("analysis")`
|
||
→ non-hex → throws → `rehydrate` throws → `resumeSession (rehydrate)` logs failure. So a
|
||
mid-session resume cannot restore produced-artifact content; downstream `needs`-injection and
|
||
artifact validation (which read `artifactContentCache`) would then be empty/broken.
|
||
- **root cause:** identical to F-007 — slot name vs CAS content-hash. The F-007 fix added
|
||
`ArtifactContentStoredEvent(artifactId=slot, contentHash=hash)` as the durable bridge, but
|
||
`rehydrate` does not consult it; it still passes the slot id straight to the hash-keyed CAS.
|
||
- **fix direction:** rehydrate from `ArtifactContentStoredEvent` — `artifactStore.get(contentHash)`
|
||
then `artifactContentCache["sessionId:slot"] = content`. The F-007/F-015 events already record
|
||
the needed slot→hash mapping; this just consumes it on cold start.
|
||
- **status:** FIXED — `rehydrate` now scans `ArtifactContentStoredEvent` (the durable slot→CAS-hash
|
||
bridge added in F-007) and calls `artifactStore.get(p.contentHash)`, keying the cache by the
|
||
logical slot name (`p.artifactId`). The slot name is never passed to the hash-keyed CAS, so the
|
||
odd-length-hex crash is gone and content is restored on cold start. `RehydrateTest` reworked to
|
||
use a content-addressed fake store (slot name ≠ content hash) so it actually exercises the bug;
|
||
both cases green. Replay (#8) unaffected — rehydrate is live-resume-only. Still wants a live
|
||
#1/#8 resume probe to confirm end-to-end restoration.
|
||
|
||
### Steering (functional contract) — partially exercised live
|
||
- During the live operator sessions, approval-with-steering was informally validated: the operator
|
||
steered toward a specific indentation; the initial attempt was made without that guidance and the
|
||
steered re-attempt incorporated it. So the steering note entered the journal and affected the
|
||
run. Not yet a controlled probe (no event-level assertion captured), but not untouched either.
|
||
|
||
## Audit outcome summary
|
||
|
||
role_pipeline is now **fully green, end-to-end**. Session `4679ed1e` (the final run) completed
|
||
`WorkflowCompleted, terminalStage=done, 5 stages` in a single clean pass — analyst → architect →
|
||
planner → implementer → reviewer → done, **no review-loop bounce**:
|
||
- the implementer followed the F-020 steering and used `file_write` (4 calls, 3 `FileWrittenEvent`s,
|
||
no `patch` botching);
|
||
- `calc.average` was correctly fixed (`if len(values) == 0: return 0.0`), real git diff, **all 7
|
||
pytest pass**;
|
||
- the `file_written` artifact carried the **diff** (F-019), the reviewer's `needs`-injection showed
|
||
`Input artifact: patch` *with the diff*, so the reviewer could actually verify the change and
|
||
**approved on the first pass**.
|
||
|
||
Every correctness defect found (F-001, F-005, F-007 through F-016, F-018, F-019, F-020) is fixed
|
||
and now confirmed live. The earlier failures (`265f5fe6` reviewer loop, `6ebe0c50` bad patch) were
|
||
the last two blockers; both are closed.
|
||
|
||
**Still to PROBE** (implementations mostly exist; the audit hasn't run the adversarial checks —
|
||
"outstanding" here means un-probed, not unimplemented):
|
||
- **#8 offline replay** — *implemented and unit-tested* (`ReplayOrchestrator` + `testing/replay/`
|
||
suite). Needs the adversarial probe (export log, replay with network/LLM off) to confirm.
|
||
- **#1 resume-after-restart** — *wiring implemented* (`POST /sessions/{id}/resume`, `resume()`,
|
||
`rehydrate()`) but **F-021 shows `rehydrate()` is broken** (slot-name-vs-CAS-hash, same as
|
||
F-007). Strongly expected to fail a live probe until F-021 is fixed.
|
||
- **#2** projection isolation, **#4** policy-vs-approval, **#6** compression non-authority,
|
||
**#9** env-observation replay — not yet probed this audit.
|
||
- **undo / cancel / approval-gate** — functional contracts not yet exercised. **Steering** was
|
||
*partially* exercised live (operator steered indentation; re-attempt incorporated it).
|
||
|
||
There is now a real recorded, validated file change (`4679ed1e`) to point the undo/replay/resume
|
||
probes at — though resume needs F-021 first. **Optional groundwork:** wire per-stage model
|
||
selection (F-020 note); the model remained the flaky element even on this passing run.
|