fix(review): handle dossier vocabulary and uncovered cohorts - #561
Conversation
Contextualize dossier process-state validation so legitimate domain vocabulary remains valid while explicit PR state and bookkeeping remain excluded. Closes #559
Preserve saved cohort reuse when no current catalog agent can cover a changed file, while retaining fresh-session guidance when a broad or matching out-of-cohort agent can cover it. Closes #562
rianjs
changed the title
PR #561 TDD / coverage assessmentVerdictNot ready: #562 misses the all-files-inherently-unassignable cohort-reuse path, so it can silently omit the required Findings
Adequate coverage / evidence
|
|
The two posted TDD findings, P1 and P2, are addressed by |
![rianjs-bot[bot]](https://avatars.githubusercontent.com/u/7378790?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: b838e2b55c21
Profile: codex-rianjs-bot - Posting as: rianjs-bot[bot]
Summary
| Reviewer | Findings |
|---|---|
| go:implementation-tests | 1 |
| policies:conventions | 0 |
| structure:repo-health | 1 |
go:implementation-tests (1 finding)
Major - internal/dossier/dossier.go:266
The contextualized lifecycle-state pattern only rejects
:/=forms, so ordinary prose such asthe session ID is 019fe123orthe run_id was abcnow passes both input filtering and summary validation into the reviewer-facing dossier. Those are still forbidden harness/session handles under the dossier contract. Extend the contextual matcher to recognize prose value predicates (for example,is/wasfollowed by a value), while retaining the explicitly tested schema/domain descriptions, and add coverage for those leaking forms.
structure:repo-health (1 finding)
Major - internal/dossier/dossier.go:274
The reviewer-facing dossier boundary still admits explicit CI process state such as
CI has failed: this pattern only recognizes forms ending infailing, while the adjacent terminal-failure patterns only cover build/check subjects. That text is neither removed from source discussion nor rejected in an LLM summary, despite the prompt contract prohibiting CI/build/process chatter. Expand the contextual state validation to cover CI terminal-state variants (and add them to the vocabulary matrix), ideally consolidating these state forms so future variants do not silently bypass both filters.
Reviewer Coverage
go:implementation-tests— complete (broad); inspected 4 assigned files (5 inspected across reviewers):internal/dossier/dossier.go,internal/dossier/dossier_test.go,internal/pipeline/pipeline.go,internal/pipeline/pipeline_test.go; skipped: none; constraints: Focused on Go implementation and behavioral-test adequacy. Package tests could not compile because clang mishandles the workspace path containing spaces.policies:conventions— complete (broad); inspected 3 assigned files (5 inspected across reviewers):docs/review-lifecycle.md,internal/pipeline/pipeline.go,internal/pipeline/pipeline_test.go; skipped: none; constraints: Reviewed assigned files for repository-local convention drift; shared cli-common and automation convenience copies were unavailable.structure:repo-health— complete (broad); inspected 4 assigned files (5 inspected across reviewers):internal/dossier/dossier.go,internal/dossier/dossier_test.go,internal/pipeline/pipeline.go,internal/pipeline/pipeline_test.go; skipped: none; constraints: Scoped to the four assigned files.go test ./internal/dossier ./internal/pipelinecould not build because clang could not resolve this workbench path.
Inspected files (5)
docs/review-lifecycle.mdinternal/dossier/dossier.gointernal/dossier/dossier_test.gointernal/pipeline/pipeline.gointernal/pipeline/pipeline_test.go
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 2m 16s | gpt-5.6-terra | cr dev
| Field | Value |
|---|---|
| Model | gpt-5.6-terra |
| Reviewers | go:implementation-tests, policies:conventions, structure:repo-health |
| Engine | codex_cli · gpt-5.6-terra |
| Reviewed by | cr · rianjs-bot[bot] |
| Duration | 2m 16s wall · 3m 56s compute |
| Cost | unavailable |
| Tokens | 732.8k in / 9.3k out |
Per-workstream usage
orchestrator-selection— gpt-5.6-terra- In: 16.8k
- Out: 332
- Cache read: 11.0k
- Cache create: unavailable
- Cost: unavailable
- Duration: 9s
go:implementation-tests— gpt-5.6-terra- In: 363.3k
- Out: 4.2k
- Cache read: 298.0k
- Cache create: unavailable
- Cost: unavailable
- Duration: 1m 44s
policies:conventions— gpt-5.6-terra- In: 74.0k
- Out: 887
- Cache read: 53.5k
- Cache create: unavailable
- Cost: unavailable
- Duration: 22s
structure:repo-health— gpt-5.6-terra- In: 242.9k
- Out: 3.4k
- Cache read: 205.1k
- Cache create: unavailable
- Cost: unavailable
- Duration: 1m 33s
orchestrator-rollup— gpt-5.6-terra- In: 35.8k
- Out: 447
- Cache read: 27.1k
- Cache create: unavailable
- Cost: unavailable
- Duration: 6s
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: dd14296e9acf
Profile: codex-rianjs-bot - Posting as: rianjs-bot[bot]
Summary
| Reviewer | Findings |
|---|---|
| go:implementation-tests | 0 |
| policies:conventions | 0 |
| structure:repo-health | 1 |
structure:repo-health (1 finding)
Major - internal/dossier/dossier.go:275
The contextual filter still lets common explicit CI/build states through:
Checks have failed.andBuild has failed.match none of lines 274-279. Since this matcher gates both raw discussion projection and generated summaries, those prohibited process states can again reach reviewer-facing dossier artifacts. Extend the terminal-state pattern across CI/check/build singular and plural forms, and add these cases to the vocabulary matrix so the boundary remains enforceable as variants are added.
Reviewer Coverage
go:implementation-tests— complete (constrained); inspected 4 assigned files (5 inspected across reviewers):internal/dossier/dossier.go,internal/dossier/dossier_test.go,internal/pipeline/pipeline.go,internal/pipeline/pipeline_test.go; skipped: none; constraints: Focused Go implementation and behavioral-test review; package test execution was not attempted in this read-only workspace.policies:conventions— complete (constrained); inspected 3 assigned files (5 inspected across reviewers):docs/review-lifecycle.md,internal/pipeline/pipeline.go,internal/pipeline/pipeline_test.go; skipped: none; constraints: Reviewed assigned files for repository-local convention drift; shared cli-common and automation convenience copies were unavailable.structure:repo-health— complete (constrained); inspected 4 assigned files (5 inspected across reviewers):internal/dossier/dossier.go,internal/dossier/dossier_test.go,internal/pipeline/pipeline.go,internal/pipeline/pipeline_test.go; skipped: none; constraints: none
Inspected files (5)
docs/review-lifecycle.mdinternal/dossier/dossier.gointernal/dossier/dossier_test.gointernal/pipeline/pipeline.gointernal/pipeline/pipeline_test.go
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 1m 15s | gpt-5.6-terra | cr dev
| Field | Value |
|---|---|
| Model | gpt-5.6-terra |
| Reviewers | go:implementation-tests, policies:conventions, structure:repo-health |
| Engine | codex_cli · gpt-5.6-terra |
| Reviewed by | cr · rianjs-bot[bot] |
| Duration | 1m 15s wall · 1m 24s compute |
| Cost | unavailable |
| Tokens | 1.1M in / 12.1k out |
Per-workstream usage
go:implementation-tests— gpt-5.6-terra- In: 526.6k
- Out: 4.9k
- Cache read: 444.9k
- Cache create: unavailable
- Cost: unavailable
- Duration: 19s
policies:conventions— gpt-5.6-terra- In: 150.5k
- Out: 1.4k
- Cache read: 116.5k
- Cache create: unavailable
- Cost: unavailable
- Duration: 15s
structure:repo-health— gpt-5.6-terra- In: 414.7k
- Out: 5.3k
- Cache read: 357.9k
- Cache create: unavailable
- Cost: unavailable
- Duration: 42s
orchestrator-rollup— gpt-5.6-terra- In: 56.3k
- Out: 519
- Cache read: 45.3k
- Cache create: unavailable
- Cost: unavailable
- Duration: 6s
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 8b3efbe3f95c
Profile: codex-rianjs-bot - Posting as: rianjs-bot[bot]
Summary
| Reviewer | Findings |
|---|---|
| go:implementation-tests | 0 |
| policies:conventions | 0 |
| structure:repo-health | 0 |
Reviewer Coverage
go:implementation-tests— complete (constrained); inspected 4 assigned files (5 inspected across reviewers):internal/dossier/dossier.go,internal/dossier/dossier_test.go,internal/pipeline/pipeline.go,internal/pipeline/pipeline_test.go; skipped: none; constraints: Focused Go implementation and behavioral-test review; package tests were not run in this read-only workspace.policies:conventions— complete (constrained); inspected 3 assigned files (5 inspected across reviewers):docs/review-lifecycle.md,internal/pipeline/pipeline.go,internal/pipeline/pipeline_test.go; skipped: none; constraints: Reviewed assigned files for repository-local convention drift; shared cli-common and automation convenience copies were unavailable.structure:repo-health— complete (constrained); inspected 4 assigned files (5 inspected across reviewers):internal/dossier/dossier.go,internal/dossier/dossier_test.go,internal/pipeline/pipeline.go,internal/pipeline/pipeline_test.go; skipped: none; constraints: none
Inspected files (5)
docs/review-lifecycle.mdinternal/dossier/dossier.gointernal/dossier/dossier_test.gointernal/pipeline/pipeline.gointernal/pipeline/pipeline_test.go
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 47s | gpt-5.6-terra | cr dev
| Field | Value |
|---|---|
| Model | gpt-5.6-terra |
| Reviewers | go:implementation-tests, policies:conventions, structure:repo-health |
| Engine | codex_cli · gpt-5.6-terra |
| Reviewed by | cr · rianjs-bot[bot] |
| Duration | 47s wall · 57s compute |
| Cost | unavailable |
| Tokens | 1.6M in / 13.9k out |
Per-workstream usage
go:implementation-tests— gpt-5.6-terra- In: 721.6k
- Out: 5.4k
- Cache read: 622.6k
- Cache create: unavailable
- Cost: unavailable
- Duration: 17s
policies:conventions— gpt-5.6-terra- In: 251.2k
- Out: 1.9k
- Cache read: 203.0k
- Cache create: unavailable
- Cost: unavailable
- Duration: 16s
structure:repo-health— gpt-5.6-terra- In: 559.3k
- Out: 6.0k
- Cache read: 485.4k
- Cache create: unavailable
- Cost: unavailable
- Duration: 19s
orchestrator-rollup— gpt-5.6-terra- In: 78.2k
- Out: 555
- Cache read: 65.5k
- Cache create: unavailable
- Cost: unavailable
- Duration: 5s
1 participant
Delivery
This is a six-commit series grouped by issue:
6c59cfbis the original contextual process-state fix;5ff5d1c,dd14296, and8b3efbeaddress the posted TDD and live-review findings while preserving domain vocabulary.f82b890is the original cohort-reuse fix;b838e2baddresses the posted TDD P1 coverage finding.The posted TDD P1/P2 findings are addressed by
b838e2band5ff5d1c; the subsequent live-review findings are addressed bydd14296and8b3efbe.Final evidence
crreview: approved, zero findings..github/workflows/ci.ymlremained explicitly unassigned.make check: green, lint 0 issues.Closes #559
Closes #562