fix(validation): resolve F-007 artifact validator CAS-hash conflation
ArtifactPayloadValidator passed the logical slot name into the content-hash-keyed CAS store, crashing every workflow with a typed produces slot. Validator now reads payloads from a content map threaded through ValidationContext (populated from the orchestrator's per-session artifact cache across all stages) and no longer depends on ArtifactStore. Also records the slot->CAS-hash mapping as a new ArtifactContentStoredEvent (registered for serialization), emitted at both CAS write sites, so artifact content is recoverable from CAS by hash after a cold start.
This commit is contained in:
@@ -0,0 +1,161 @@
|
||||
# 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.
|
||||
|
||||
## 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:** confirmed
|
||||
|
||||
### 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:** confirmed
|
||||
|
||||
### 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:** confirmed
|
||||
Reference in New Issue
Block a user