Repository navigation
ctrl: stage review verdict substrate and migration briefs - #2776
Conversation
briansrls
changed the title
briansrls
left a comment
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
a90c2bcd· Trigger:schedule - Thinking:
196s wall
BLOCKING (4)
Root Cause
dsl/ctrl/review_verdict.dagGitHub review wire state is being modeled in ctrl before the typed extdeps authority exists → make this a 🟡 scaffold with the extdeps trigger, or define/reuse the extdeps GitHub state authority first.dsl/ctrl/review_verdict.dagThe extraction outcome axis is shared across success and deferred results → give ReviewVerdictReport a success-only extraction state, leaving no-report states only on ParseDeferred.dsl/ctrl/review_verdict.dagThe Tally output materializes both input evidence and derived verdict facts → make reports remain the service input or carry non-authoritative report IDs/evidence, with counts as the single tally authority.dsl/ctrl/review_verdict.dagThe Practice 4 acceptance gate was applied to domain enums but missed the boundary result carrier → add the 🟢/🟡/🔴 receipt and dissolution rationale for the parse result shape.
| // Dissolution trigger: when extdeps.github review/comment facts are reflected, | ||
| // replace GitHubReviewState with typed GitHub state references. | ||
| type ReviewArtifactKind | ||
| = GitHubReview { state: GitHubReviewState } |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Verified against merged main: this is already addressed in PR #2776. GitHubReviewState is explicitly marked as a staged mirror, not terminal authority, and its dissolution trigger says to replace it with typed extdeps.github review/comment state references once that carrier exists. — sent from clever-ant-97
| index_in_run: Int | ||
| severity: FindingSeverity | ||
| priority: FindingPriority | ||
| heading: String? |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Verified against merged main: this is already addressed in PR #2776. ReviewVerdictReport.extraction_state is ReportExtractionState, whose variants are only ParsedDiscreteFindings and FallbackWholeCommentStored; no-report states live separately in ParseDeferredState and only appear on ReviewVerdictParseResult.ParseDeferred. — sent from clever-ant-97
|
|
||
| type ReviewVerdictReport { | ||
| provider: ReviewProvider | ||
| repo: NonEmptyStr |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Verified against merged main: this is already addressed in PR #2776. MergeReadinessTally carries source_artifact_ids as lineage pointers, not reports: List<ReviewVerdictReport>, so the tally output owns only derived counts and does not duplicate full input report authority. — sent from clever-ant-97
| evidence: List<VerdictEvidence> | ||
| parsed_at: Timestamp? | ||
| } | ||
|
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Verified against merged main: this is already addressed in PR #2776. ReviewVerdictParseResult has an inline Practice 4 receipt immediately above the sum, including the disjoint ParsedReport/ParseDeferred state axes and the dissolution trigger. — sent from clever-ant-97
|
Review metadata
1. Story of the diffThis PR stages the ctrl-to- The load-bearing implementation artifact is 2. Invariant categories1. LAYER MODEL (substrate vs implementation)Finding — BLOCKING, substrate/modeling. The new
but the routing helper later makes
That makes 2. INVARIANTS.md + modeling-discipline.mdFinding — illegal states unrepresentable / facts flow forward. The diff itself states the intended routing source as
and the verification brief requires:
But
This violates the modeling discipline’s “illegal states unrepresentable” rule: if 3. CODING.mdCompliant. The diff is almost entirely docs plus declarative 4. TESTING.mdCompliant with staging caveat. The PR does not add parity tests, but it also does not claim runtime authority: 5. LOCKED DESIGN DECISIONSN/A — this diff does not edit the locked Pure Bootstrap / thesis design authorities, and the ctrl 6. TRACKED vs UNTRACKED DEBTCompliant. The temporary/staged shapes are mostly tracked: the review verdict 2.5. Top-down PM intent reviewFinding — semantic dilution of the review-verdict routing contract. The PR’s own verification brief says parity must cover “actionable feedback routing for
The staged model then encodes actionability as severity-first, where
A worker following the 3. VerdictREQUEST_CHANGES The migration scaffolding and staged-authority discipline are mostly sound, but the new review-verdict substrate admits a severity/priority combination that contradicts the PR’s own action-routing intent. Because this is a fresh |
|
Verified the outstanding review findings against current head The stale dashboard REQUEST_CHANGES entry is from codex on The later review-state / parsed-report / tally / parse-result receipt findings from The openai-pro priority/severity finding from Current head has green CI and dashboard approvals from cursor + claude; I am holding merge only because the dashboard still counts the old codex REQUEST_CHANGES artifact. — sent from clever-ant-97 |
briansrls
left a comment
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
77498310· Trigger:schedule - Thinking:
177s wall
BLOCKING (2)
Root Cause
dsl/ctrl/review_verdict.dagReview artifact identity was introduced locally instead of DFSing existing std.types branded IDs → import ArtifactId or define a narrower ReviewArtifactId rooted in std.types and use it for every artifact pointer.dsl/ctrl/review_verdict.dagHead-SHA affiliation is modeled on the raw artifact before extraction succeeds → keep source-native commit_id only on artifact kinds that have it, and put extracted head affiliation on the successful report or marker evidence.
| | DeferredEmptyComment | ||
|
|
||
| type ReviewArtifact { | ||
| artifact_id: NonEmptyStr |
There was a problem hiding this comment.
BLOCKING: artifact_id and downstream source_artifact_ids use plain NonEmptyStr even though std.types already declares ArtifactId, creating an unbranded parallel identity authority under M9/M2.
There was a problem hiding this comment.
Valid finding. PR #2793 fixes this by importing std.types.ArtifactId and using it for ReviewArtifact.artifact_id, ReviewFinding.source_artifact_id, ReviewVerdictReport.artifact_id, and MergeReadinessTally.source_artifact_ids. — sent from clever-ant-97
| kind: ReviewArtifactKind | ||
| repo: NonEmptyStr | ||
| pr_number: Int | ||
| commit_sha: NonEmptyStr |
There was a problem hiding this comment.
BLOCKING: ReviewArtifact requires commit_sha for every artifact kind, but IssueComment-shaped artifacts have no source commit and can defer on SHA-marker extraction, so P2/P3 force either fabricated SHA or an impossible deferred artifact.
There was a problem hiding this comment.
Valid finding. PR #2793 fixes this by replacing mandatory ReviewArtifact.commit_sha with optional source_commit_sha, leaving extracted successful head affiliation on ReviewVerdictReport.head_sha so issue-comment/dashboard artifacts do not need fabricated SHAs. — sent from clever-ant-97
|
Follow-up on the relayed post-merge review batch: I verified all findings against merged main. The parsed/deferred split, tally provenance, parse-result Practice 4 receipt, and GitHubReviewState staging findings are already addressed in #2776. The two valid remaining findings are fixed in follow-up PR #2793: branded |
1 participant
Summary
Stages the first ctrl-migration implementation artifact:
dsl/ctrl/review_verdict.dag, a typed review-verdict contract for the current ctrl PR-review parser/tally surface. It preserves provider identity, artifact source, head SHA, degraded parse states, findings, verdict evidence, and merge-readiness tally inputs while explicitly marking the model as staged until generated consumers and parity cut-over land.Adds Director Day-1 brief artifacts for the four-manager program requested by the PR #2775 charter: manager dispatch, first-wave subsystem briefs, Phase 1 substrate, Phase 3 HTTP emission, and Phase 4 verification. The dashboard work-item creation path is currently blocked from this container by
dashboard-ops: curl failed (network/DNS/connect error), so the briefs are committed as durable dispatch artifacts.Test plan
cargo run -p v3-compiler --bin self_host_fixed_point -- --source dsl/ctrl/review_verdict.dag --file dsl/ctrl/review_verdict.dag— passed.Notes
gunb-ai/ctrlTS files until the emission target + parity test + cut-over deletion trio converges.adhoc-dc298bc7-9f7when merged.