Repository navigation
proud-gull-252 - #901
proud-gull-252#901
Conversation
|
Manager review — keep this draft; do not mark ready yet. This PR is currently the same head SHA as closed #895 (
Please push a real corrective commit on this worker branch or close this duplicate draft. Reopening the same blocked head under a new PR number does not satisfy the ready/close audit. |
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
6f494af6· Trigger:schedule - Thinking:
370s wall
BLOCKING (6)
Root Cause
dsl/extdeps/github/auth.dagSecret Manager token acquisition lacks GitHub scope/expiry evidence → keep the raw token carrier or model known/unknown scope and expiry evidence explicitly.dsl/extdeps/llm/anthropic.dagAnthropic request messages need provider-specific role and content-block wire carriers → model the internally type-discriminated block products before replacing Json.dsl/extdeps/llm/openai.dagOpenAI chat messages are a provider-specific union keyed by role with different required fields → model those variants instead of a single product over the shared Role.
|
Verification vs blocking inline review (relay may reference older SHA
Full before/after table and citations are in the updated PR description. |
|
Re: inline at Verified on current HEAD of
The relay text matches an older revision where |
…roles - OpenAI: extend OpenAiChatMessageRole with Developer per Chat Completions API; add structural_coverage_gap for tool/multimodal message shapes not modeled. - Anthropic: note content-block wire vs emitted serde may diverge; add structural_coverage_gap_anthropic_content_block_request_wire for alignment. Addresses PR #901 review: wire fidelity is gap-carried, not silent; Role claim on OpenAiChatMessage was already false at head (OpenAiChatMessageRole). Made-with: Cursor
|
Scheduled api-review ( That review predates the landed corrections. Root-cause mapping at
Earlier thread at Net: direction matches review; blocking items from |
|
Worker verify (current Matches your note; no code change. Line anchors drifted after gap-receipt edits: |
|
Worker cross-check vs table (HEAD Re-read sources: all four disposition rows and the Spot-check: |
|
Re: line anchors ( Substance unchanged. Tiny anchor fix: Still not |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0a98c73ae5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Second-pass verify (same HEAD Re-grepped / re-read: the disposition table + spot-check in the parent comment still match (Cross-check: |
|
Re: anchor fix thread — re-verify Pulled |
…ages Codex P1: typed REST bodies still rely on default serde for unit enums and sum types; provider wire expects string roles and flat type-keyed content objects. Replace underspecified gap row with explicit receipts for role + content (Anthropic) and role strings (OpenAI); same closure trigger rest_request_wire_serde_alignment. Made-with: Cursor
|
Re: Codex inline Finding is directionally correct: We already carried a generic gap;
So: not dismissed as stale — it is a real wire/serde alignment debt, explicitly gap-named (not patched with effect tags). Full fix belongs in |
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
dc113a17· Trigger:schedule - Thinking:
260s wall
BLOCKING (2)
Root Cause
dsl/extdeps/llm/anthropic.dagprovider role closed set was introduced as substrate without a dissolution receipt → add a terminal/scaffold/dissolve classification comment at the declaration.dsl/extdeps/llm/openai.dagprovider role closed set was introduced as substrate without a dissolution receipt → add a terminal/scaffold/dissolve classification comment at the declaration.
Non-blocking — Strengths
dsl/extdeps/llm/anthropic.dagThe Messages role/system split matches https://docs.anthropic.com/en/api/messages and the remaining request-wire serde gap is named.dsl/extdeps/llm/openai.dagThe narrow text-message carrier includes developer and names the missing full-surface Chat Completions cases from https://platform.openai.com/docs/api-reference/chat/create-chat-completion.
|
Manager disposition for queued review on current head The two queued blockers are valid and narrowly scoped. Expected shape: classify each role set explicitly rather than leaving reviewers to infer it. If they are terminal provider wire/domain authority for the currently modeled narrow request surface, say that. Also preserve the existing distinction that request wire serialization remains gap-carried under No broader extdeps rewrite is requested for this review item. |
|
Addressed the current blocking role-coproduct classification feedback in b60bd69. Verification:
Checks: |
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
b60bd69b· Trigger:schedule - Thinking:
298s wall
Non-blocking — Strengths
dsl/extdeps/github/github.dagThe PAT carrier split preserves extdeps fidelity by refusing to treat Secret Manager token bytes as GitHub-issued scope or expiry facts.
✅ No blocking concerns in the current PR patch.
|
Review metadata
1. Story of the diffThis PR tightens a few external-dependency request models by replacing loose or overbroad shapes with provider-specific carriers. GitHub secret-manager tokens stop looking like full GitHub-issued auth tokens: The PR also migrates two consumers off the old JSON-string bridge: the review tool now declares 2. Invariant categories
BLOCKING — P2 Boundary Discipline / substrate modeling:
BLOCKING — illegal states unrepresentable / single-authority metadata: the new call sites now have to manually restate the discriminator with
N/A — this diff is almost entirely
The diff updates an existing behavior fixture to exercise the new typed Anthropic message input (
N/A — the diff does not appear to alter a locked thesis/design decision; it names residual wire-serde gaps rather than claiming those gaps are closed.
BLOCKING — untracked scaffold inside the model: the coverage-gap rows are tracked well for response typing and request-wire serde ( 3. VerdictREQUEST_CHANGES The direction is right: provider-specific typed message inputs and explicit serde-gap receipts are better than opaque |
|
Subsumed this PR and worked through the queued review feedback. Current state at :
No additional code changes after ; the remaining action is reviewer/merge decision. |
|
Corrected status summary for #901: Current state at
No additional code changes after |
|
Disposition for the scheduled Codex finding at 2026-04-26T23:11:54Z on 6484954: verified against the merged tree. The source facts are accurate: dsl/extdeps/llm/anthropic.dag still has Messages.messages as List, dsl/gunbc/tools/review.dag still routes review_messages through llm.Anthropic.Messages, and structural_coverage_gap_anthropic_messages_request_wire still names the role-string / flat type-keyed content serialization residue under rest_request_wire_serde_alignment. No worker fix commit was pushed because #901 was merged at 2026-04-26T23:26:35Z, and the immediately following director disposition explicitly approved this shape for merge as typed-primitive consumption with the request-wire residue carried as structural-coverage-gap data. The remaining full fix is the named follow-up closure rest_request_wire_serde_alignment, not an untracked blocker on the now-merged PR. |
|
Disposition for the director approval queued at 2026-04-26T23:12:58Z: verified against current merged #901 state. The approval matches the code: GitHub PAT materialization is split into extdeps/github/auth.dag without fabricated scopes/expiry; Anthropic/OpenAI message coproducts carry terminal provider-domain classification comments; residues are named as structural coverage gaps with closure triggers rest_typed_response_body, rest_request_wire_serde_alignment, and anthropic_tool_result_full_content_surface. #901 is already merged at 6484954 via merge commit 12bc8c4, with fmt, ci, v3, and self_host_ratchet green. No fix commit is warranted for this item; it is an approval/merge directive and has been carried out. |
|
Disposition for dashboard-delivered echoes at 2026-04-26T23:28:46Z and 2026-04-26T23:29:16Z: no new source finding. Verified current #901 state again: PR is merged at 6484954 via merge commit 12bc8c4, with fmt, ci, v3, and self_host_ratchet green. The 23:28:46 item is my already-posted disposition for the scheduled Codex request-wire finding; the 23:29:16 item is my already-posted disposition for the director approval/merge directive. No fix commit or further PR action is warranted from these echoed comments. |
|
Review metadata
Verdict: APPROVE — Diff narrowly replaces Highlights (not findings):
Exploratory observation (non-ask): the request-wire serde gap is now declared in two places (anthropic + openai) referencing the same closure name — once that closure lands, both gap rows dissolve together. Worth keeping the closure name stable so a single ratchet sweep retires both. |
Single control surface for cleanup-lane harvest of #900/#901/#920/#897/#824/#825 per dispatch from tidy-dove-734 (#941). No ctrl#263 in repo; this docs/audit artifact is the agreed fallback. Rows: source PR, gap, file/invariant, owner lane, dissolution trigger, acceptance check, tracking authority, disposition.
* docs(audit): W-C1 follow-up harvest table Single control surface for cleanup-lane harvest of #900/#901/#920/#897/#824/#825 per dispatch from tidy-dove-734 (#941). No ctrl#263 in repo; this docs/audit artifact is the agreed fallback. Rows: source PR, gap, file/invariant, owner lane, dissolution trigger, acceptance check, tracking authority, disposition. * docs(audit): close #897/#824/#825 rows per bright-wolf-465 audit Per bright-wolf-465 (inbox #945), no missed BLOCKING findings on these PRs; flip rows 6/7/8 to closed with audit citation. * docs(audit): reconcile note with row dispositions for #897/#824/#825 * WIP: calm-ant-861 * chore: apply cargo fmt * WIP: calm-ant-861 * fix(test): fold B5 Loop closure receipt into m1_substrate_test SG-0 census ratchet forbids new hand-authored .rs files. Move the every_loop_node_originates_from_recursive_function_lowering test (and the LoopNode/LoopBound import + two helper fns) into the existing m1_substrate_test.rs and drop the standalone file + its integration.rs registration. Behavior unchanged. * docs: correct B5 receipt file path in synthesis doc (m1_substrate_test, not standalone file) * docs: mark ROADMAP loop-emission row resolved + correct synthesis cite Codex BLOCKING on be4a6ab: ROADMAP.md still listed the loop-emission semantic invariant as open debt while the synthesis doc declared it resolved — tracker-authority mismatch. - ROADMAP.md:436: rewrite the row as RESOLVED 2026-04-27 with PR cite, audit summary (two production sites in lower.rs), receipt location (m1_substrate_test.rs), and marker-retired note. - synthesis-doc §3 line 155: fix the receipt path (loop_construction_closure_test.rs → m1_substrate_test.rs after the SG-0 fold). * fix(test): split global vs fixture-scoped Loop closure assertions Codex BLOCKING on db7b773: previous test mixed a global all-Dag closure check with fixture-only variant claims, so bootstrap loops could mask the fixture-coverage claim. Split into two phases: - Global: every Behavior::Loop in the Dag (fixture + bootstrap) satisfies the recursive-function-lowering signature (loop.output is a Bind value port + Descent cluster id resolves). This is the closure invariant. - Fixture-scoped: filter loops by span.file == fixture file before claiming both LoopBound variants are produced by THIS fixture. Bootstrap loops are excluded so the variant claim is mechanically about the fixture's recursive functions. * post-merge: drop superseded artifacts; correct ROADMAP B5 receipt cite The parallel B5 lane landed `r2_b5_loop_construction_closure_test.rs` on main with a more rigorous receipt (substring push-site ratchet + per-fixture DAG walk + Origin::Accumulated provenance). The harvest table in this PR is also being landed via aggregate #949. Drop: - `docs/audit/w-c1-followup-harvest-2026-04-27.md` (canonical surface is #949). - The m1_substrate_test additions (auto-reverted via merge --theirs; superseded by the standalone r2_b5_loop_construction_closure_test.rs on main). Keep: - `ROADMAP.md` row marking loop-emission RESOLVED, with citation rewritten to point at the actual landed receipt file (r2_b5_…) instead of the retired m1_substrate_test cite.
Add checkable pr-825-pre-merge-review-audit reconciling codex/claude/openai-pro findings against main. ROADMAP subsection names the three remaining #901 closure triggers (rest_typed_response_body, openai_chat_message_full_coproduct, anthropic_tool_result_full_content_surface) with receipt pointers; rest_request_wire_serde_alignment noted separately. Made-with: Cursor
* docs(std.unicode): cite UCD 15.x / UAX-11 authority Closes the #920 post-merge citation gap. Header now states the file is sourced from UCD 15.x (UAX #11 East Asian Width) for the display- width tables and is intentionally 15.x compatible rather than pinned to a specific minor. UAX #9 is explicitly not consulted (no bidi). No behavior changes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs: tighten P5 per-PR dissolution gate (b) for ROADMAP-cited deferrals Require exactly one checkable receipt: delete path, SG-0 census before/after counts, or lane plus concrete ROADMAP row/link. Call out vague deferrals as insufficient. Align INVARIANTS §P5 (b) with the template without duplicating the checklist. Made-with: Cursor * docs(audit): W-C1 follow-up harvest table Single control surface for cleanup-lane harvest of #900/#901/#920/#897/#824/#825 per dispatch from tidy-dove-734 (#941). No ctrl#263 in repo; this docs/audit artifact is the agreed fallback. Rows: source PR, gap, file/invariant, owner lane, dissolution trigger, acceptance check, tracking authority, disposition. * docs(audit): mark #920 citation follow-up closed * docs(audit): close #897 #824 #825 harvest rows * docs(audit): cite closure evidence for harvest rows * WIP: Cleanup * docs(std.unicode): clarify UAX 11 coverage * docs: link cleanup harvest from roadmap * docs(std.unicode): refresh bootstrap spans * docs(std.unicode): refresh full bootstrap spans * docs(std.unicode): refresh no-parse bootstrap spans * docs(audit): stabilize harvest code references --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
PR #901 bullet 3 now names rest_request_wire_serde_alignment_receipt paid rows and structural_coverage_gap_anthropic_tool_result_nested_block_wire_payloads; retired staging gap identifiers called out explicitly. Frontmatter roadmap row in r3-coproduct-2 brief matches. Co-authored-by: Cursor <cursoragent@cursor.com>
…t-2; do not reuse closed PR #2646 (#2711) * WIP: T-Anthropic-Wire slice 2 recovery — redo #2606 from brief r3-coproduct-2 * WIP: T-Anthropic-Wire slice 2 recovery — redo #2606 from brief r3-coproduct-2 * WIP: T-Anthropic-Wire slice 2 recovery — redo #2606 from brief r3-coproduct-2 * WIP: T-Anthropic-Wire slice 2 recovery — redo #2606 from brief r3-coproduct-2 * WIP: T-Anthropic-Wire slice 2 recovery — redo #2606 from brief r3-coproduct-2 * WIP: T-Anthropic-Wire slice 2 recovery — redo #2606 from brief r3-coproduct-2 * WIP: T-Anthropic-Wire slice 2 recovery — redo #2606 from brief r3-coproduct-2 * chore: apply cargo fmt to anthropic integration tests Co-authored-by: Cursor <cursoragent@cursor.com> * WIP: T-Anthropic-Wire slice 2 recovery — redo #2606 from brief r3-coproduct-2 * WIP: T-Anthropic-Wire slice 2 recovery — redo #2606 from brief r3-coproduct-2 * docs(INVARIANTS): P5 SG-0 receipt for anthropic_tool_result_wire_demo_test Register the hand-authored integration test path under P5 Dispatch-Discipline (b) with a checkable ROADMAP citation, dissolution trigger, and pointers to the bootstrap projection ratchet + nested-block residual gap rows. Co-authored-by: Cursor <cursoragent@cursor.com> * WIP: T-Anthropic-Wire slice 2 recovery — redo #2606 from brief r3-coproduct-2 * WIP: T-Anthropic-Wire slice 2 recovery — redo #2606 from brief r3-coproduct-2 * docs: align ROADMAP/brief with live anthropic tool-result receipts PR #901 bullet 3 now names rest_request_wire_serde_alignment_receipt paid rows and structural_coverage_gap_anthropic_tool_result_nested_block_wire_payloads; retired staging gap identifiers called out explicitly. Frontmatter roadmap row in r3-coproduct-2 brief matches. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(anthropic): singleton wire tag for fixed text discriminators Replace loose String slots on AnthropicToolResultTextBlock.type and AnthropicToolResultPlainTextDocumentSource.type with AnthropicToolResultWireTextTag (Text {}) per P2 / modeling discipline. Mirror in v3 anthropic_schema.dag, regen bootstrap snapshots, add disj lockstep for the new carrier. Co-authored-by: Cursor <cursoragent@cursor.com> * chore(ci): retrigger after SG-0 PR body pairing lines Co-authored-by: Cursor <cursoragent@cursor.com> * WIP: T-Anthropic-Wire slice 2 recovery — redo #2606 from brief r3-coproduct-2 * WIP: T-Anthropic-Wire slice 2 recovery — redo #2606 from brief r3-coproduct-2 * fix(anthropic): enumerate nested tool-result SDK payload gaps in residual Expand structural_coverage_gap_anthropic_tool_result_nested_block_wire_payloads with explicit closure rows for TextBlockParam (cache_control, citations), ImageBlockParam and ToolReferenceBlockParam (cache_control), and CacheControlEphemeralParam.ttl vs stringly CacheControl — per blocking review on tool_result nested fidelity (anthropic-sdk-python OpenAPI/Stainless). Co-authored-by: Cursor <cursoragent@cursor.com> * WIP: T-Anthropic-Wire slice 2 recovery — redo #2606 from brief r3-coproduct-2 --------- Co-authored-by: Cursor <cursoragent@cursor.com>
Authority
OperationEffect/SideEffects/readonly/idempotenttags on this lane).Shared Rule For Residues
P1 Bypass Before / After
ChatCompletionmessages: Jsonplus output path extractionmessages: List<OpenAiChatMessage>with `OpenAiChatMessageRole = SystemMessagesmessages: Jsonplus output path extractionmessages: List<AnthropicChatMessage>where the outer variant is the role discriminator:UserMessage { content: List<AnthropicUserContentBlock> }orAssistantMessage { content: List<AnthropicAssistantContentBlock> }. User rows can carry text/tool results; assistant rows can carry text/tool-use requests. Wireroleandtypekeys are not caller-authored model state.github_tokenGitHubSecretManagerPat { token }only. The carrier now lives indsl/extdeps/github/auth.dagwith the Secret Manager acquisition function, not in the GitHub core provider model. Noscopes: []and noexpires_at: none; unknown metadata is not encoded as observed-empty metadata.Gap Receipts
OpenAI / Anthropic outputs still use
from "..."path projection. This slice names that residue as queryable structural-coverage-gap data:structural_coverage_gap_openai_chat_completion_outputsindsl/extdeps/llm/openai.dagstructural_coverage_gap_anthropic_messages_outputsindsl/extdeps/llm/anthropic.dagClosure trigger:
rest_typed_response_body.Request wire serialization is also named explicitly because the Rust emitter's default enum/sum serde may not match provider wire contracts:
structural_coverage_gap_openai_chat_request_messages_wirestructural_coverage_gap_anthropic_messages_request_wireClosure trigger:
rest_request_wire_serde_alignment.Anthropic message/content variants deliberately do not expose caller-writable
roleortypediscriminator fields.structural_coverage_gap_anthropic_messages_request_wirenames the remaining obligation: the future serde alignment closure must emit role strings and flat contenttypewire keys fromUserMessage/AssistantMessageand their role-specific content variants.Anthropic tool-result content is intentionally a narrow text-result slice in this PR. Non-string/nested/image tool-result content is explicitly gap-carried by
structural_coverage_gap_anthropic_tool_result_full_content_surfacewith closure triggeranthropic_tool_result_full_content_surface.Coproduct Classification
AnthropicChatMessageis classified as a 🟢 terminal provider-domain coproduct for modeled Messages row roles.AnthropicUserContentBlockandAnthropicAssistantContentBlockare classified as 🟢 terminal provider-domain coproducts for role-legal content rows in the modeled slice.OpenAiChatMessageRoleis classified as a 🟢 terminal provider-domain coproduct for the modeled narrow text-message row.TextBlock / Content Blocks
Anthropic user text is modeled as
UserTextBlock { text: String }; assistant text is modeled asAssistantTextBlock { text: String }. Tool results are currentlyUserToolResultBlock { tool_use_id, content: String, is_error }, with richer tool-result content gap-carried. Call sites indsl/gunbc/tools/review.dagandsrc/v2/tests/src/pipeline.rsuseUserMessage { content: [UserTextBlock { text: ... }] }. The wire"type":"text"discriminator belongs torest_request_wire_serde_alignment.Optional
Timestamp?/noneNot used on the Secret Manager PAT path after the carrier split. Elsewhere,
nonefor optionals matches existing repo precedent.Verification
git diff --checkpassed after the latest Secret Manager carrier / tool-result gap fix.cargois not installed. GitHub Actions should supplyfmt,ci,v3, andself_host_ratchetreceipts for this PR.