fix(threadanalysis): accept thread analysis output that omits schema_version - #605
Conversation
…version Smaller models return well-formed, semantically correct thread analysis JSON without the schema_version constant, and the decoder rejected it outright. A single rejected thread fails the whole analysis task blocking, discarding an entire review run over a field whose only legal value is 1. Absent schema_version now defaults to the current version; an explicitly wrong version is still rejected. The prompt also names schema_version inside the field list rather than alongside it, so the constant reads as required output rather than as an aside. The pipeline resume test's thread-analysis sentinel keyed off the old prompt wording and would have silently stopped matching; it now keys off the prompt's purpose line.
monit-reviewer
left a comment
monit-reviewer
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 9bce7da3b165
Profile: pi-ds-gh - Posting as: monit-reviewer
Summary
| Reviewer | Findings |
|---|---|
| go:implementation-tests | 0 |
| structure:repo-health | 2 |
structure:repo-health (2 findings)
Minor - internal/threadanalysis/threadanalysis.go:284
The loosened check (
raw.SchemaVersion != 0 && raw.SchemaVersion != outputSchemaVersion) tests absence against the currentoutputSchemaVersion, andresultOutput.SchemaVersionis a plainint. Two compounding effects: (1) an explicit"schema_version": 0is indistinguishable from an omitted field and is accepted, even though 0 is never a legal version — the new table covers absent/1/2/99 but not 0; (2) the next timeoutputSchemaVersionis bumped to 2, output with noschema_versionis silently validated as v2 with no deliberate decision, test, or review gate forcing that choice, because the tolerance is defined relative to the current version rather than a fixed known set. It is also now the only structured-output boundary in the repo that tolerates absence:llm.DecodeFindings(internal/llm/contracts.go:242) still requires equality, andinternal/approvaloverride/approvaloverride.go:120,133uses*intspecifically to make presence required. So a repo-wide policy decision ("model may omit schema_version") currently lives in one code comment and is likely to be copied as a pattern with no stated intent.Concrete fix: make absence explicit instead of collapsing it into the zero value — decode into
SchemaVersion *intand resolve via a named helper (e.g.effectiveSchemaVersion(), accepting nil or the documented known versions), reject explicit 0, and add explicit-0 plus a simulated version-bump case toTestDecodeResultSchemaVersion. If tolerance is the intended policy, record it where durable task contracts already live (docs/llm-task-artifacts.md, "Thread Analysis Tasks") so the next version bump has a written rule to check against.
Minor - internal/pipeline/pipeline_test.go:829
TestLiveResumeRecoversPostedThreadSummaryForReviewerPromptstill answers "did the resumed run repeat thread analysis?" by substring-matching prose owned by another package (internal/threadanalysis's first prompt line), and it is a negative assertion. If that literal ever stops appearing in the prompt, the loop simply finds nothing and the test keeps passing — the guard goes dead silently, which is the exact failure mode this PR documents for the old sentinel. Nothing in the suite pins the new literal to the real prompt (TestPromptForInputEnumeratesSchemaVersionpins only the fields line), so this coupling can rot again with no signal.Concrete fix: assert on request identity instead of prose. The captured
llm.RequestexposesLogPath, and thread-analysis requests log under the owned, versioned pathrunartifact.Paths.ThreadAnalysisLogsDir()(<agent-logs>/thread-analysis/<encoded-thread>.jsonl), while reviewer/rollup requests do not — e.g.strings.HasPrefix(request.LogPath, filepath.Join(artifacts.AgentLogsDir, "thread-analysis")+string(filepath.Separator))alongside the existinglen(requests) != 2check. If the prose sentinel is kept, add a positive control so a reword fails loudly: assert the same literal is present in the first adapter's thread-analysis request prompt before asserting the second adapter never produced it.
Reviewer Coverage
go:implementation-tests— complete (constrained); skipped: none; constraints: Repo-wide literal searches were used to confirm the new pipeline sentinel is unique to the thread-analysis prompt; I did not inspect the pipeline's prompt builders line-by-line. Review is limited to the three assigned changed files; supporting reads were read-only context (internal/llm/fake.go, docs/development.md). Tests were not executed in this environment; mutation-check claims in the PR body were verified by reasoning about the assertions, not by running the mutations.structure:repo-health— complete (constrained); skipped: none; constraints: Assigned scope was the three changed files; sibling boundary decoders (internal/llm/contracts.go, internal/approvaloverride/approvaloverride.go) and runartifact paths were inspected only as comparison evidence. Read windows are byte-ranged in this harness, so line anchors were confirmed against CR search line numbers rather than whole-file reads. Read-only review: no build or test execution, so all claims are from inspection of the diff, the changed files, and adjacent decoders/paths.
Inspected files (3)
internal/pipeline/pipeline_test.gointernal/threadanalysis/threadanalysis.gointernal/threadanalysis/threadanalysis_test.go
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 2m 32s | $0.01 | opencode-go/deepseek-v4.1-flash | cr dev
| Field | Value |
|---|---|
| Model | opencode-go/deepseek-v4.1-flash |
| Reviewers | go:implementation-tests, structure:repo-health |
| Engine | pi_rpc · opencode-go/deepseek-v4.1-flash |
| Reviewed by | cr · monit-reviewer |
| Duration | 2m 32s wall · 3m 37s compute |
| Cost | $0.01 |
| Tokens | 12.2k in / 6.8k out |
Per-workstream usage
orchestrator-selection— opencode-go/deepseek-v4.1-flash- In: 6.3k
- Out: 1.3k
- Cache read: 0
- Cache create: 0
- Cost: $0.00
- Duration: 7s
go:implementation-tests— opencode-go/deepseek-v4.1-flash- In: 200
- Out: 3.4k
- Cache read: 35.2k
- Cache create: 0
- Cost: $0.00
- Duration: 1m 20s
structure:repo-health— opencode-go/deepseek-v4.1-flash- In: 435
- Out: 1.9k
- Cache read: 50.9k
- Cache create: 0
- Cost: $0.00
- Duration: 2m 06s
orchestrator-rollup— opencode-go/deepseek-v4.1-flash- In: 5.3k
- Out: 283
- Cache read: 0
- Cache create: 0
- Cost: $0.00
- Duration: 2s
…e resume guard to log identity Absence was encoded as the zero value, so an explicit "schema_version": 0 was indistinguishable from an omitted field and was accepted even though 0 is never a legal version. Decode into *int and resolve through a named helper, matching the presence idiom already used in approvaloverride; absent still resolves to the current version, explicit 0 is now rejected. The pipeline resume guard asserted that a resumed run does not repeat thread analysis by substring-matching a prompt literal owned by another package. As a negative assertion it dies silently whenever that prose changes. It now matches on the thread-analysis agent-log directory, which the run layout owns.
monit-reviewer
left a comment
monit-reviewer
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: c36e83d17520
Profile: pi-ds-gh - Posting as: monit-reviewer
Summary
| Reviewer | Findings |
|---|---|
| go:implementation-tests | 1 |
| structure:repo-health | 1 |
go:implementation-tests (1 finding)
Minor - internal/threadanalysis/threadanalysis.go:277
resolveOutputSchemaVersionsplits one decision across two places: it rejects an explicit0here, but returns every other wrong value (2,99, negative) fordecodeResultForThreadto reject with its own identical-format error at line 299. The explicit-zero branch has no distinct behavior — because the field is now*int,nilalready encodes "absent", and the caller'sversion != outputSchemaVersionwould reject0with the exact same message (schema_version = 0, want 1). So the branch is unreachable in practice, dupes the caller's error construction, and creates an inconsistent contract (0 is validated inside the helper, other wrong versions outside it), which invites the next maintainer to assume 0 is special. Collapse it to a pure resolver and let the single comparison own rejection:func resolveOutputSchemaVersion(raw *int) int { if raw == nil { return outputSchemaVersion } return *raw }
TestDecodeResultSchemaVersionis unaffected —explicit_zero_schema_version_is_rejectedstill fails through the caller's comparison with the same substring.
structure:repo-health (1 finding)
Minor - internal/pipeline/pipeline_test.go:830
Invariant: this test is the only guard that a resumed run does not re-issue thread analysis, so it must fail loudly whenever the resumed run does re-issue it. The repaired check is still a negative-only assertion:
strings.HasPrefix(request.LogPath, threadAnalysisLogsDir+string(filepath.Separator))is false for every request whoseLogPathis empty, so ifLogPathever stops being populated for reviewer/rollup tasks (adapter or task-wiring change), the loop asserts nothing and the test passes while the resumed pipeline quietly repeats thread analysis. That is the same silently-dead-guard failure mode the PR set out to fix, just moved from prompt prose to a path convention. Today the guard's discriminating power is established by a one-off manual mutation (repointing the prefix atAgentLogsDir); that evidence is not reproducible in CI and will not survive the next refactor. Fix (about 3 lines): assert positively that each request log path lies under the agent-log root before the negative check, e.g.agentLogsDir := ArtifactPathsFromDir(run.ArtifactPath).AgentLogsDirthen inside the loopif !strings.HasPrefix(request.LogPath, agentLogsDir+string(filepath.Separator)) { t.Fatalf("request log path %q outside agent log dir", request.LogPath) }.AgentLogsDiris a true prefix of both captured paths (they areagent-logs/<encoded>.jsonl) and also a prefix ofagent-logs/thread-analysis/..., so it passes here but still proves the negative check is evaluated against real values.
Reviewer Coverage
go:implementation-tests— complete (constrained); skipped: none; constraints: I did not execute go test/go build; assessment of the new tests is from reading the decoder, prompt, and test bodies, not observed runs. Settled discussion outcomes were treated as resolved and not re-litigated; only new, concrete Go/test observations are reported. cr_diff was read first; scope was limited to the three assigned files plus the read-only call sites needed to judge them (runartifact.Paths, llm.FakeAdapter/Request, threadrespond LogPath wiring, pipeline analyzeReviewThreads).structure:repo-health— complete (constrained); skipped: none; constraints: Scope: reviewed the three assigned changed files plus read-only context (internal/runartifact, internal/llm/fake.go, internal/threadrespond, docs/llm-task-artifacts.md). I did not run go test; the author's build/test claims were not re-verified. The absent-schema_version-means-current rule is still undocumented in docs/llm-task-artifacts.md ("Thread Analysis Tasks"). That gap was already raised and explicitly deferred in the settled review thread, so I am not re-raising it as a finding.
Inspected files (3)
internal/pipeline/pipeline_test.gointernal/threadanalysis/threadanalysis.gointernal/threadanalysis/threadanalysis_test.go
2 PR discussion threads considered. 2 summarized; 2 resolved.
Completed in 3m 28s | $0.00 | opencode-go/deepseek-v4.1-flash | cr dev
| Field | Value |
|---|---|
| Model | opencode-go/deepseek-v4.1-flash |
| Reviewers | go:implementation-tests, structure:repo-health |
| Engine | pi_rpc · opencode-go/deepseek-v4.1-flash |
| Reviewed by | cr · monit-reviewer |
| Duration | 3m 28s wall · 4m 24s compute |
| Cost | $0.00 |
| Tokens | 6.7k in / 4.8k out |
Per-workstream usage
go:implementation-tests— opencode-go/deepseek-v4.1-flash- In: 502
- Out: 3.6k
- Cache read: 50.0k
- Cache create: 0
- Cost: $0.00
- Duration: 2m 44s
structure:repo-health— opencode-go/deepseek-v4.1-flash- In: 1.0k
- Out: 980
- Cache read: 38.9k
- Cache create: 0
- Cost: $0.00
- Duration: 1m 36s
orchestrator-rollup— opencode-go/deepseek-v4.1-flash- In: 5.2k
- Out: 231
- Cache read: 0
- Cache create: 0
- Cost: $0.00
- Duration: 2s
…tive control to the resume guard resolveOutputSchemaVersion rejected an explicit zero itself while every other wrong version was rejected by the caller, splitting one decision across two places and duplicating the error. Since the field is a pointer, nil already encodes absence, so the helper is now a pure resolver and the caller's single comparison owns rejection. The resume guard was negative-only: a request whose log path went missing would match nothing and pass silently. Each captured request is now asserted to live under the agent-log root before the thread-analysis check runs, so the negative assertion is proven to evaluate against real values in CI rather than by hand.
monit-reviewer
left a comment
monit-reviewer
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 9e62114cb231
Profile: pi-ds-gh - Posting as: monit-reviewer
Summary
| Reviewer | Findings |
|---|---|
| go:implementation-tests | 1 |
| structure:repo-health | 1 |
go:implementation-tests (1 finding)
Nits - internal/threadanalysis/threadanalysis.go:414
The prompt restates the schema version as a literal —
schema_version (always 1)— whileoutputSchemaVersionis the authority the decoder compares against one function away. The new test has to paper over the duplication:TestPromptForInputEnumeratesSchemaVersionrebuilds the whole production sentence withfmt.Sprintf("Return JSON with fields: schema_version (always %d), ...", outputSchemaVersion), so prompt text and constant are now kept in sync by hand, and any rephrasing of this non-load-bearing sentence forces an edit to the copied literal in the test.Concrete fix: interpolate the constant in the prompt (
fmt.Sprintf("Return JSON with fields: schema_version (always %d), thread_id, decision, reply_body, summary, resolve, rationale.", outputSchemaVersion)) and have the test assert a shorter stable anchor (theschema_version (always <version>)token plus the field list) instead of a copy of the production string.Non-blocking: a version bump still trips the test today, so this is a duplication/maintenance nit rather than a silent-failure path. The rest of the change (
*intfor absence, pureresolveOutputSchemaVersion, single rejection site with one consistent error, table coverage for absent/0/1/2/99, and the pipeline guard's positive control onAgentLogsDirbefore the negativethread-analysis/check) is idiomatic and adequately tested for its risk.
structure:repo-health (1 finding)
Minor - internal/threadanalysis/threadanalysis.go:291
The decoder now resolves the version (
version := resolveOutputSchemaVersion(raw.SchemaVersion)) but drops it:Result.SchemaVersion(line 481) is never assigned anywhere in the repo — theResultliteral at line 295 omits it and no package sets or reads it (repo-wideSchemaVersionsearch shows only the declaration). So the exported struct thatthreadrespond.Analysescarries always exposes the zero value, while the wire type now declaresnilas "current version". Invariant: a value resolved and validated at a boundary should either be surfaced on the result or not exist on the struct. Impact: the two representations of the same contract disagree, and any future consumer that gates onResult.SchemaVersion(replayed/persisted analyses, a future version bump, dossier or ledger rows) silently reads 0 with nothing in the test suite catching it. Fix, one line either way: setSchemaVersion: versionin theResultliteral at line 295, or delete the unused field. Not introduced by this diff, but this is the change that makes the value available and the natural place to reconcile it.
Reviewer Coverage
go:implementation-tests— complete (constrained); skipped: none; constraints: Read-only review: I did not executego buildorgo test, so the PR's mutation-check and liveness evidence was not independently reproduced; findings are based on reading the diff and surrounding code. Scope stayed on the three assigned files plus the seams they touch:internal/runartifactpaths (AgentLogsDir,ThreadAnalysisLogsDir,ThreadAnalysisLog),llm.FakeAdapterrequest/resume capture, and the pipeline thread-analysis log-path call site. cr_read ranged responses here are byte-offset based (with byte line numbers from cr_search), so for the 372KBinternal/pipeline/pipeline_test.goI inspected the changed test region (lines ~729-880) rather than the entire file.structure:repo-health— complete (constrained); skipped: none; constraints: Scope limited to the three assigned files; approvaloverride, dossier, llm, runartifact, threadrespond were only spot-checked through cr_search to compare schema_version handling. cr_read returned truncated fragments of every requested range (only the tail of the window), so file contents were established from cr_diff plus cr_search line matches rather than full-file reads.
Inspected files (3)
internal/pipeline/pipeline_test.gointernal/threadanalysis/threadanalysis.gointernal/threadanalysis/threadanalysis_test.go
2 PR discussion threads considered. 2 summarized; 2 resolved.
Completed in 2m 31s | $0.01 | opencode-go/deepseek-v4.1-flash | cr dev
| Field | Value |
|---|---|
| Model | opencode-go/deepseek-v4.1-flash |
| Reviewers | go:implementation-tests, structure:repo-health |
| Engine | pi_rpc · opencode-go/deepseek-v4.1-flash |
| Reviewed by | cr · monit-reviewer |
| Duration | 2m 31s wall · 3m 45s compute |
| Cost | $0.01 |
| Tokens | 6.2k in / 8.5k out |
Per-workstream usage
go:implementation-tests— opencode-go/deepseek-v4.1-flash- In: 768
- Out: 7.0k
- Cache read: 52.7k
- Cache create: 0
- Cost: $0.00
- Duration: 1m 53s
structure:repo-health— opencode-go/deepseek-v4.1-flash- In: 219
- Out: 1.2k
- Cache read: 34.9k
- Cache create: 0
- Cost: $0.00
- Duration: 1m 48s
orchestrator-rollup— opencode-go/deepseek-v4.1-flash- In: 5.2k
- Out: 255
- Cache read: 0
- Cache create: 0
- Cost: $0.00
- Duration: 2s
| "Analyze this inline code-review discussion thread.", | ||
| "Return JSON only. Do not include markdown fences or prose outside JSON.", | ||
| "Use schema_version 1 and fields: thread_id, decision, reply_body, summary, resolve, rationale.", | ||
| "Return JSON with fields: schema_version (always 1), thread_id, decision, reply_body, summary, resolve, rationale.", |
There was a problem hiding this comment.
The prompt restates the schema version as a literal — schema_version (always 1) — while outputSchemaVersion is the authority the decoder compares against one function away. The new test has to paper over the duplication: TestPromptForInputEnumeratesSchemaVersion rebuilds the whole production sentence with fmt.Sprintf("Return JSON with fields: schema_version (always %d), ...", outputSchemaVersion), so prompt text and constant are now kept in sync by hand, and any rephrasing of this non-load-bearing sentence forces an edit to the copied literal in the test.
Concrete fix: interpolate the constant in the prompt (fmt.Sprintf("Return JSON with fields: schema_version (always %d), thread_id, decision, reply_body, summary, resolve, rationale.", outputSchemaVersion)) and have the test assert a shorter stable anchor (the schema_version (always <version>) token plus the field list) instead of a copy of the production string.
Non-blocking: a version bump still trips the test today, so this is a duplication/maintenance nit rather than a silent-failure path. The rest of the change (*int for absence, pure resolveOutputSchemaVersion, single rejection site with one consistent error, table coverage for absent/0/1/2/99, and the pipeline guard's positive control on AgentLogsDir before the negative thread-analysis/ check) is idiomatic and adequately tested for its risk.
Reply inline to this comment.
| } | ||
| if raw.SchemaVersion != outputSchemaVersion { | ||
| return Result{}, fmt.Errorf("threadanalysis: schema_version = %d, want %d", raw.SchemaVersion, outputSchemaVersion) | ||
| version := resolveOutputSchemaVersion(raw.SchemaVersion) |
There was a problem hiding this comment.
The decoder now resolves the version (version := resolveOutputSchemaVersion(raw.SchemaVersion)) but drops it: Result.SchemaVersion (line 481) is never assigned anywhere in the repo — the Result literal at line 295 omits it and no package sets or reads it (repo-wide SchemaVersion search shows only the declaration). So the exported struct that threadrespond.Analyses carries always exposes the zero value, while the wire type now declares nil as "current version". Invariant: a value resolved and validated at a boundary should either be surfaced on the result or not exist on the struct. Impact: the two representations of the same contract disagree, and any future consumer that gates on Result.SchemaVersion (replayed/persisted analyses, a future version bump, dossier or ledger rows) silently reads 0 with nothing in the test suite catching it. Fix, one line either way: set SchemaVersion: version in the Result literal at line 295, or delete the unused field. Not introduced by this diff, but this is the change that makes the value available and the natural place to reconcile it.
Reply inline to this comment.
…onstant The prompt hardcoded the version while outputSchemaVersion was the value the decoder compared against, so the test had to mirror the whole production sentence to keep the two in sync by hand. The prompt now interpolates the constant and the test asserts a stable anchor plus the field names, so a version bump no longer requires editing a copied literal.
monit-reviewer
left a comment
monit-reviewer
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 06be0ba32581
Profile: pi-ds-gh - Posting as: monit-reviewer
Summary
| Reviewer | Findings |
|---|---|
| go:implementation-tests | 1 |
| structure:repo-health | 1 |
go:implementation-tests (1 finding)
Minor - internal/threadanalysis/threadanalysis_test.go:442
This loop is the only guard that
promptForInput's field list stays complete, but four of its six assertions are already satisfied by unrelated prompt text, so it cannot fail for the reason it exists.
promptForInputappends the marshalledanalysisInputcontext JSON, which contains"schema_version","thread_id"and"resolved", and the fixed output-contract lines include"skip: reply_body and summary must be empty; resolve must be false.". Sostrings.Contains(prompt, "thread_id"),"reply_body","summary"and"resolve"all pass even if those names are removed from the enumeratedReturn JSON with fields: ...line. Onlydecisionandrationale(and the separately anchoredschema_version (always %d)check) actually bind the instruction line.Impact: deleting
resolveorthread_idfrom the field list leaves CI green — a guard that silently stops guarding, the same rot class this PR repaired inpipeline_test.go. The wording now claims the enum is locked while half of it is not.Fix: scope the assertion to the instruction line instead of the whole prompt, and derive the expected names from the struct that defines them rather than repeating them. Extract the line first, then check each
resultOutputjson tag appears in it:line := promptForInputFieldsLine(t, prompt) // e.g. first line starting "Return JSON with fields:" for _, field := range []string{"schema_version", "thread_id", "decision", "reply_body", "summary", "resolve", "rationale"} { if !strings.Contains(line, field) { t.Fatalf("field list line missing %q: %q", field, line) } }Holding the field list in a package constant shared by
promptForInputand the test is an acceptable smaller variant; either way the assertion must not be satisfiable by the embedded thread context or the contract prose.
structure:repo-health (1 finding)
Minor - internal/threadanalysis/threadanalysis.go:272
Invariant: the schema-version policy for durable LLM task payloads is owned by
docs/llm-task-artifacts.md.docs/development.mdpoints at that file as the source of truth for "the artifact schema, status taxonomy, and resume invariants", and itsSchema Version/Resume Rulessections already state how version mismatches fail closed.Risk: this diff changes a load-bearing boundary contract for a
failed_blockingtask (an absent outputschema_versionis now accepted and resolved tooutputSchemaVersion) but records the rule only in code and PR text. The repo already carries three divergent treatments of the same boundary question:internal/threadanalysis/threadanalysis.go(nil-> current, explicit0rejected, viaresolveOutputSchemaVersion),internal/approvaloverride/approvaloverride.go:132(absent rejected as "required"), andinternal/dossier/dossier.go:736(zero value tolerated as absent). Nothing versioned says which of these is the intended policy for a given task, so the next author adding a structured decoder, or bumpingoutputSchemaVersionto2, has no written rule to check against and will very plausibly invent a fourth variant. That is exactly the compounding drift the doc'sSchema Versionsection exists to prevent.Impact: a future
outputSchemaVersionbump silently accepts absent-version payloads as the new version (absence always maps to current, never to the version the model actually targeted), and reviewers of that bump get no signal that the resolver must be revisited. Scope is bounded to thread analysis today, but the ambiguity is repo-wide.Fix: add two sentences to the
Thread Analysis Taskssection (or alongside theSchema Versionsection) ofdocs/llm-task-artifacts.mdrecording the rule the decoder now implements: absentschema_versionresolves to the currentoutputSchemaVersion, an explicit0or any other value is rejected asschema_version = X, want N, the prompt instructs presence but presence is not required, and any bump ofoutputSchemaVersionmust revisitresolveOutputSchemaVersion. If the policy should instead be uniform across tasks, say that too, so theapprovaloverrideanddossiervariants become deliberate exceptions rather than coincidence.
Reviewer Coverage
go:implementation-tests— complete (constrained); skipped: none; constraints: Out-of-scope for this agent: the deferreddocs/llm-task-artifacts.mdnote and the version-bump policy were already settled in the review threads and are not re-raised. Read-only harness: nogo test/go buildexecuted, so all test-behavior statements are static analysis of the diff and surrounding code, not observed runs. Repo file reads return character windows, so files were sampled around changed regions plus adjacent call sites (runartifact.Paths,llmlifecycle,llm.RunStructuredWithSessionResume) rather than read end to end. Schema-version tolerance was verified to have a single decode site (decodeResultForThread); llmlifecycle and llm re-use the same decoder, so no second gate was assumed.structure:repo-health— complete (constrained); skipped: none; constraints: Cross-package evidence for the schema-version concern was read outside the assigned changed files (internal/dossier/dossier.go, internal/approvaloverride/approvaloverride.go, docs/llm-task-artifacts.md); no findings are anchored there. Read-only review: no shell or test execution was available, so I could not rungo testor the mutations cited in the PR/thread evidence; behavior claims rest on inspection of head files plus the recorded discussion. Two inline threads already settled the pipeline_test resume-sentinel and the resultOutput schema-version helper details; I deliberately did not re-raise those and focused on contract ownership and durability.
Inspected files (3)
internal/pipeline/pipeline_test.gointernal/threadanalysis/threadanalysis.gointernal/threadanalysis/threadanalysis_test.go
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 2m 38s | $0.01 | opencode-go/deepseek-v4.1-flash | cr dev
| Field | Value |
|---|---|
| Model | opencode-go/deepseek-v4.1-flash |
| Reviewers | go:implementation-tests, structure:repo-health |
| Engine | pi_rpc · opencode-go/deepseek-v4.1-flash |
| Reviewed by | cr · monit-reviewer |
| Duration | 2m 38s wall · 4m 31s compute |
| Cost | $0.01 |
| Tokens | 6.5k in / 6.5k out |
Per-workstream usage
go:implementation-tests— opencode-go/deepseek-v4.1-flash- In: 352
- Out: 3.2k
- Cache read: 49.7k
- Cache create: 0
- Cost: $0.00
- Duration: 2m 15s
structure:repo-health— opencode-go/deepseek-v4.1-flash- In: 549
- Out: 3.1k
- Cache read: 51.2k
- Cache create: 0
- Cost: $0.00
- Duration: 2m 13s
orchestrator-rollup— opencode-go/deepseek-v4.1-flash- In: 5.6k
- Out: 164
- Cache read: 0
- Cache create: 0
- Cost: $0.00
- Duration: 2s
| if !strings.Contains(prompt, anchor) { | ||
| t.Fatalf("prompt missing schema_version anchor %q:\n%s", anchor, prompt) | ||
| } | ||
| for _, field := range []string{"thread_id", "decision", "reply_body", "summary", "resolve", "rationale"} { |
There was a problem hiding this comment.
This loop is the only guard that promptForInput's field list stays complete, but four of its six assertions are already satisfied by unrelated prompt text, so it cannot fail for the reason it exists.
promptForInput appends the marshalled analysisInput context JSON, which contains "schema_version", "thread_id" and "resolved", and the fixed output-contract lines include "skip: reply_body and summary must be empty; resolve must be false.". So strings.Contains(prompt, "thread_id"), "reply_body", "summary" and "resolve" all pass even if those names are removed from the enumerated Return JSON with fields: ... line. Only decision and rationale (and the separately anchored schema_version (always %d) check) actually bind the instruction line.
Impact: deleting resolve or thread_id from the field list leaves CI green — a guard that silently stops guarding, the same rot class this PR repaired in pipeline_test.go. The wording now claims the enum is locked while half of it is not.
Fix: scope the assertion to the instruction line instead of the whole prompt, and derive the expected names from the struct that defines them rather than repeating them. Extract the line first, then check each resultOutput json tag appears in it:
line := promptForInputFieldsLine(t, prompt) // e.g. first line starting "Return JSON with fields:"
for _, field := range []string{"schema_version", "thread_id", "decision", "reply_body", "summary", "resolve", "rationale"} {
if !strings.Contains(line, field) {
t.Fatalf("field list line missing %q: %q", field, line)
}
}Holding the field list in a package constant shared by promptForInput and the test is an acceptable smaller variant; either way the assertion must not be satisfiable by the embedded thread context or the contract prose.
Reply inline to this comment.
| } | ||
|
|
||
| // resolveOutputSchemaVersion maps an absent schema_version to the current version. | ||
| func resolveOutputSchemaVersion(raw *int) int { |
There was a problem hiding this comment.
Invariant: the schema-version policy for durable LLM task payloads is owned by docs/llm-task-artifacts.md. docs/development.md points at that file as the source of truth for "the artifact schema, status taxonomy, and resume invariants", and its Schema Version / Resume Rules sections already state how version mismatches fail closed.
Risk: this diff changes a load-bearing boundary contract for a failed_blocking task (an absent output schema_version is now accepted and resolved to outputSchemaVersion) but records the rule only in code and PR text. The repo already carries three divergent treatments of the same boundary question: internal/threadanalysis/threadanalysis.go (nil -> current, explicit 0 rejected, via resolveOutputSchemaVersion), internal/approvaloverride/approvaloverride.go:132 (absent rejected as "required"), and internal/dossier/dossier.go:736 (zero value tolerated as absent). Nothing versioned says which of these is the intended policy for a given task, so the next author adding a structured decoder, or bumping outputSchemaVersion to 2, has no written rule to check against and will very plausibly invent a fourth variant. That is exactly the compounding drift the doc's Schema Version section exists to prevent.
Impact: a future outputSchemaVersion bump silently accepts absent-version payloads as the new version (absence always maps to current, never to the version the model actually targeted), and reviewers of that bump get no signal that the resolver must be revisited. Scope is bounded to thread analysis today, but the ambiguity is repo-wide.
Fix: add two sentences to the Thread Analysis Tasks section (or alongside the Schema Version section) of docs/llm-task-artifacts.md recording the rule the decoder now implements: absent schema_version resolves to the current outputSchemaVersion, an explicit 0 or any other value is rejected as schema_version = X, want N, the prompt instructs presence but presence is not required, and any bump of outputSchemaVersion must revisit resolveOutputSchemaVersion. If the policy should instead be uniform across tasks, say that too, so the approvaloverride and dossier variants become deliberate exceptions rather than coincidence.
Reply inline to this comment.
2 participants
Closes #603
Problem
Thread analysis rejects well-formed, semantically correct JSON when the model omits
schema_version. Because one failed thread analysis isfailed_blocking, an entire review run is discarded over a field whose only legal value is1.Change
schema_versiondefaults to the current version. An explicitly wrong version is still rejected.schema_versioninside the field list instead of alongside it, so the constant reads as required output rather than as an aside.TestLiveResumeRecoversPostedThreadSummaryForReviewerPromptasserted a resumed run does not repeat thread analysis by matching the old prompt wording. That sentinel would have silently stopped matching, leaving the guard dead; it now keys off the prompt's purpose line, which is stable.Evidence
Each assertion was mutation-checked — the mutation is applied, the named test is shown failing, then reverted.
!= outputSchemaVersion)TestDecodeResultSchemaVersion/absent_schema_version_defaults_to_currentif false).../explicit_future_...and.../explicit_far-future_schema_version_is_rejectedTestPromptForInputEnumeratesSchemaVersionTestLiveResumeRecoversPostedThreadSummaryForReviewerPromptThe last row proves the repaired guard is evaluated against real prompts; with the old sentinel the test passes no matter what, because no prompt contains that string any more.
go build ./...andgo test -count=1 ./...are clean.