Repository navigation
feat: RE-3/RE-4 — Anthropic REST backend + emission fixes - #384
Conversation
ChatGPT ReviewI reviewed the PR diff. Overall: good RE-3/RE-4 progress, but I would not merge this as-is. I see two blocking issues and a few medium concerns.
In diff lines 5–12, chatgpt-review-c5aa3312-d5b5-4f… chatgpt-review-3c5f1390-4fe5-47…
In chatgpt-review-c5aa3312-d5b5-4f…
The PR hardcodes chatgpt-review-54575df2-e175-45…
In diff lines 294–308, chatgpt-review-c5aa3312-d5b5-4f…
In A couple of modeling notes on the
On roadmap alignment:
chatgpt-review-c5aa3312-d5b5-4f… My merge bar here would be: fix #1 and #2 before merge, and ideally clean up #3 in the same PR since it is a straightforward single-authority correction. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 82ef1e310f
ℹ️ 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".
| type: UserType | ||
| // Raw string: GitHub returns "User", "Bot", "Organization". | ||
| // Enum deserialization needs serde variant renames (future emitter work). | ||
| type: String |
There was a problem hiding this comment.
Changing GitHubUser.type to String makes the current PR mock responses invalid, because extdeps/github/pulls.dag still emits type: { _variant: "HumanUser" } (e.g., in List and Get mocks). In dry-run mode those mocks are deserialized via serde_json::from_value(...), so any path that materializes PullRequest.user.type will now fail at runtime with a map-vs-string type error instead of returning mock data.
Useful? React with 👍 / 👎.
| command = backend_command(backend: backend, model: model, prompt_file: prompt_file, cwd: "/Users/briansrls/gunbc") | ||
| llm_result = shell.Exec.Run( | ||
| script: "mkdir -p /Users/briansrls/.local/share/gunbc-review && printf '%s' '{prompt}' > {prompt_file} && {command}" | ||
| llm_response = llm.Anthropic.Messages( |
There was a problem hiding this comment.
review_pr still accepts a backend parameter (and review_cycle passes it through), but this code unconditionally calls llm.Anthropic.Messages. That means callers who set backend to another value now get silently routed to Anthropic and can fail due to missing ANTHROPIC_API_KEY, which is a behavioral regression from the prior backend-switching flow.
Useful? React with 👍 / 👎.
ChatGPT ReviewI reviewed the PR diff. Overall: good RE-3/RE-4 progress, but I would not merge this as-is. I see two blocking issues and a few medium concerns.
In diff lines 5–12, chatgpt-review-c5aa3312-d5b5-4f… chatgpt-review-3c5f1390-4fe5-47…
In chatgpt-review-c5aa3312-d5b5-4f…
The PR hardcodes chatgpt-review-54575df2-e175-45…
In diff lines 294–308, chatgpt-review-c5aa3312-d5b5-4f…
In A couple of modeling notes on the
On roadmap alignment:
chatgpt-review-c5aa3312-d5b5-4f… My merge bar here would be: fix #1 and #2 before merge, and ideally clean up #3 in the same PR since it is a straightforward single-authority correction. |
ChatGPT ReviewI reviewed the PR diff and kept the review scoped to the diff itself, using the attached docs only as authority for invariants/patterns. Overall: good direction for RE-3/RE-4, but I would not merge this as-is. I see two clear blockers and a few medium concerns.
In the diff, There is a second problem in the same seam: chatgpt-review-b64ee721-da56-42… chatgpt-review-7f0b5430-d295-43… chatgpt-review-54575df2-e175-45…
That looks like structure loss in emit being papered over in tests instead of fixed at the source. The invariants explicitly warn against downstream semantic repair and fabricated fallbacks, and the modeling guide’s preferred direction for anonymous products is “name them before emission”, not “quietly collapse them into tuples/arrays”. chatgpt-review-f9af4385-6df3-43… chatgpt-review-c5aa3312-d5b5-4f… chatgpt-review-8e90beda-f7a3-44…
The switch from shell/CLI to chatgpt-review-54575df2-e175-45… There is also a smaller smell here:
The PR hardcodes
The new string-response path is an improvement over blindly using chatgpt-review-c5aa3312-d5b5-4f…
Directionally this is clearly Track 5 work, and the choice not to edit roadmap status is the right one. The attached roadmap still says RE-2 is done, RE-3 is partial, and RE-4 is not started. The new tests here do not change that: they are ignored source-audit/emit-smoke tests, not live integration, and the invariant docs still say Tier 3 full-real integration is not yet implemented. So this PR helps Track 5, but it does not justify a status bump by itself. chatgpt-review-7f0b5430-d295-43… chatgpt-review-f9af4385-6df3-43… A few positives:
My merge bar would be: fix the |
ChatGPT ReviewI reviewed the diff itself and treated I would not merge this as-is. Two issues look blocking, and a few others are meaningful follow-ups. 1. Blocker — The new
chatgpt-review-ad2fe2f7-46aa-4d… chatgpt-review-ad2fe2f7-46aa-4d… The fix I’d want is either per-type/per-enum wire contracts, or a richer wire model that distinguishes at least:
Right now the model is too coarse, and the emitter is compensating. 2. Blocker — the new shell multi-field emission path fabricates values by field-name heuristic. In
That happens in the new branch added around the shell-return hunk in That is exactly the class of problem the invariants warn about: if emit needs the fact “which shell channel populates which output field,” that fact belongs upstream in the model/IR, not as name-based guessing in Rust emission. The current branch is a fabrication fallback disguised as convenience. chatgpt-review-e569fb71-e239-43… INVARIANTS I’d push this back into modeled transport semantics, for example with explicit field sources like 3. Medium —
and the file comment says modules declare: But none of the changed extdep modules do that. They all declare That is a straightforward single-authority problem. Either use chatgpt-review-ad2fe2f7-46aa-4d… 4. Medium — The new I would want response-body format to be explicit in the transport/response model, not inferred in 5. Medium — the RE-2 CLI output contract appears to regress from named JSON to positional JSON. In That may be acceptable internally, but it is a user-visible contract change. If anything consumes 6. Testing / roadmap alignment — this is RE-4 plumbing, not RE-4 completion.
That is useful, but it is not Tier 3 full-real integration. The invariants’ testing model explicitly distinguishes dry-run/structural checks from full-real live-service execution, and says Tier 3 full-real integration is not yet implemented. So this PR clearly advances RE-4 infrastructure, but it does not prove “Anthropic REST API end-to-end” yet. chatgpt-review-2d3c421a-7f02-4c… chatgpt-review-e569fb71-e239-43… A few things I do like in the diff:
The smallest acceptable merge shape for me would be:
Everything else I’d mark as follow-up rather than hard stop. |
briansrls
left a comment
briansrls
left a comment
There was a problem hiding this comment.
codex · gpt-5.4 · b576a97e
Review (INVARIANTS: 4, MODELING: 5+/5-, ROADMAP: 1✓/2!)
INVARIANTS — Violations (4)
ROOT CAUSE ANALYSIS
dsl/std/serialization.dagMissing upstream wire-shape authority (M1/M3/M7/M9):std.serializationintroduces bothWireContractand a directVariantEncodingpath, then the emitter collapses the surviving module-level fact to one string attribute for all enums; replace that with one structural wire-form authority on each relevant type or request/response shape, such as unit-string enum vs tagged object union, and have the emitter read that plusLanguageSpec.serializationmechanically.src/v2/05_emit_rust.dagMissing upstream response-body-format fact (M1/M3/M7): service responses currently say only status-to-type, so emit falls back to a type-name heuristic to decide transport decoding; add an explicit response body kind in the service model, for example raw text versus JSON-decoded value, so emission becomes a lookup instead of a branch on string-ness.src/v2/05_emit_rust.dagMissing upstream shell extraction model (M1/M5/M8): shell outputs do not carry structural facts about which field comes from stdout, stderr, or exit status, so emit guesses by name and coproduct shape; introduce typed shell field sources, analogous to RESTfrom_keyextraction, so the graph saysfrom_stdoutorfrom_exit_statusand the emitter only renders declared structure.
MODELING — Strengths
dsl/gunbc/tools/review.dagReplacing the CLI shell-out with a direct import ofextdeps.llm.anthropicis the right L5 tool composition over an L3 provider service.dsl/extdeps/github/github.dagUserType = User | Bot | Organizationis a more faithful closed set than the inventedHumanUserlabel.dsl/extdeps/llm/llm.dagRole = System | User | Assistant | Toolis a clean shared-provider L2 closed set and composes better than provider-prefixed variants.dsl/extdeps/llm/anthropic.dagauth_source: EnvVarplus the explicitanthropic-versionheader captures real API contract facts at the provider-specific service layer.dsl/std/serialization.dagVariantNamingandVariantEncodinguse closed coproducts instead of string flags, which is the right M1/M4 shape for wire-format facts.
MODELING — Improvements
dsl/gunbc/tools/review.dagThe Anthropic request is still assembled as raw JSON and prompt strings; compose it from shared message/content types so backend behavior stays structural rather than stringly (M1/M8, Track 10).dsl/extdeps/github/github.dagKeep wire-shape facts onUserTypeitself instead of a module-widewire_contract, because the same file also defines other enums with different wire meanings (M3/M4).dsl/extdeps/llm/llm.dagIf this file owns provider-shared wire facts,LlmMessageshould also move toward provider-shared content blocks rather thanString; otherwise keep serialization contracts in L3 provider modules (M1/M3, Track 10).dsl/extdeps/llm/anthropic.dagA module-wideSnakeCaseenum contract is not faithful for mixed Anthropic types such asContentBlockandAnthropicModel; model each wire form on the relevant type or request/response boundary instead (M1/M3).dsl/std/serialization.dagEither makeWireContractthe single consumed authority or delete it; as added here it is parallel scaffolding at the wrong granularity (M2/M7/M9).
ROADMAP — Verified
- RE-2: review.dag compiles (dry-run): The diff still supports RE-2 by building and dry-running
review-prwith an Anthropic env placeholder in the pipeline test.
ROADMAP — Incomplete
- RE-3: review.dag passes live integration: The runtime path changed substantially, but the diff adds no non-ignored full-real review-cycle test proving the tool completes against live services.
- RE-4: Anthropic REST API end-to-end: The added RE-4 tests are ignored compile/emission checks and dry-run never exercises the live Anthropic API, so end-to-end integration is still not demonstrated.
ROADMAP — Unclaimed
- RE-4: Anthropic REST API end-to-end: This PR clearly starts RE-4 by adding Anthropic auth, endpoint/header plumbing, and targeted emission coverage, but
ROADMAP.mdstill saysNot started.
The PR makes real RE-4 progress and includes some strong modeling fixes, but the new wire-contract and emitter heuristics still violate core single-authority and no-fabrication invariants.
| // TaggedVariant → #[serde(tag = "_variant")] (default) | ||
| fn resolve_wire_serde_tag(wire_item: Node) -> String { | ||
| // wire_item body is a VariantEncoding value directly (StringVariant or TaggedVariant) | ||
| match wire_item.body { |
There was a problem hiding this comment.
Invariant violation: resolve_wire_serde_tag reads wire_contract as bare VariantEncoding, so the new WireContract boundary never becomes authority, violating the single-authority metadata and end-to-end semantic-boundary invariants.
| // Resolve module-level wire contract for enum serde attributes. | ||
| // Modules with wire_contract: WireContract get StringVariant or TaggedVariant; | ||
| // modules without get the default _variant tagging. | ||
| let serde_enum_tag = match typed_module.items |> filter(i => i.name == "wire_contract") |> first { |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| let resp_value = field_init_node_value(n: prop) | ||
| // Response type reference may not be fully resolved — check both | ||
| // inferred type and the source-level name via coercion data. | ||
| let is_string_response = match resp_value.inferred { |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| concat("let parsed: serde_json::Value = serde_json::from_str(&stdout)?;\nOk(serde_json::from_value(parsed)?)") | ||
| // Shell multi-field: construct tuple from process output channels. | ||
| // Map each field by name: success→exit status, stdout→stdout, stderr→stderr. | ||
| let field_exprs = effective.children |> map(ch => |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Review (INVARIANTS: 4, MODELING: 5+/5-, ROADMAP: 1✓/2!) INVARIANTS — Violations (4) ROOT CAUSE ANALYSIS
MODELING — Strengths
MODELING — Improvements
ROADMAP — Verified
ROADMAP — Incomplete
ROADMAP — Unclaimed
The PR makes real RE-4 progress and includes some strong modeling fixes, but the new wire-contract and emitter heuristics still violate core single-authority and no-fabrication invariants. |
Addresses invariant violation feedback on PR #384: 1. Wire contract: no longer collapses to module-wide string. Passes wire_contract_item (Node?) per-enum for per-type resolution. 2. Response format (text vs json): reverted is_string_like heuristic. Needs WireFormat fact on response declarations in std/. 3. Shell multi-field return: reverted name-based channel fabrication. Needs channel→field mapping in transport definition. Both #2 and #3 now emit compile_error! on the unmodeled cases, making the gap explicit rather than silently fabricating. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ChatGPT ReviewI reviewed the diff itself and treated My take: not merge-ready. I see two blockers and a few meaningful follow-ups. 1. Invariant violationsBlocker:
|
briansrls
left a comment
briansrls
left a comment
There was a problem hiding this comment.
codex · gpt-5.4 · 75fbf4c0
Review (INVARIANTS: 3, MODELING: 2+/2-, ROADMAP: 0✓/1!)
INVARIANTS — Violations (3)
ROOT CAUSE ANALYSIS
dsl/extdeps/llm/anthropic.dagThe upstream gap is that llm.Anthropic.Messages exposes messages as generic Json instead of a structural request type, so review.dag compensates by hand-encoding JSON text and that text flows unchanged into the transport body; define AnthropicMessage or AnthropicRequest at L3, reuse LlmMessage where valid, and make callers pass data rather than encoded strings per M1, M8, and M9.dsl/extdeps/llm/anthropic.dagThe upstream gap is that service transport declarations cannot yet consume version authority as structure, so the version fact is declared once as data and again as raw header strings; add a structural api_version field or allow header expressions to reference current_api_version, then derive the emitted header from that single authority per M7 and M9.src/v2/05_emit_rust.dagThe upstream gap is that shell transports still have no structural channel-to-field mapping, so emit cannot know whether each product field comes from stdout, stderr, exit status, or parsed JSON and compensates with a generated compile_error!; model shell output channels in the transport or type layer and make lowering or inference carry that mapping so emit only translates it, aligned with M1, M8, and M9.
MODELING — Strengths
dsl/std/serialization.dagVariantNaming, VariantEncoding, and WireContract put enum wire-shape facts in a reusable L0 vocabulary instead of scattering target-specific cases through extdeps modules and emitters.dsl/extdeps/github/github.dagRenaming UserType to User, Bot, and Organization matches GitHub's actual wire values and improves extdeps fidelity without inventing a new abstraction.
MODELING — Improvements
dsl/extdeps/llm/anthropic.dagMessages.input.messages: Json is too weak for the Anthropic spec; introduce a structural Anthropic request message type so tool code composes message arrays from data rather than encoded text.dsl/extdeps/llm/llm.dagA module-level snake_case wire_contract in the provider-agnostic LLM vocabulary overstates cross-provider commonality; keep the shared conversation types neutral at L2 and let Anthropic and OpenAI declare their concrete wire mappings at L3.
ROADMAP — Incomplete
- RE-3: review.dag passes live integration: The new live path still constructs the Anthropic messages field as encoded text, so the review agent is not at a reliable end-to-end integration point yet.
ROADMAP — Unclaimed
- RE-4: Anthropic REST API end-to-end: The diff clearly advances RE-4 with a provider module, auth_source, header emission, review-tool integration, and tests, but ROADMAP.md still says Not started and the current request-modeling bug means only Partial is justified.
The PR makes real RE-4 progress and removes one prior shell-emission fabrication, but it still has a malformed Anthropic request body, a duplicated version authority, and a new emit-stage compile_error fallback.
| llm_response = llm.Anthropic.Messages( | ||
| api_key: "", | ||
| model: model, | ||
| messages: "[{\"role\": \"user\", \"content\": \"Review the PR described in the system prompt.\"}]", |
There was a problem hiding this comment.
Invariant violation: messages is passed as a JSON-encoded string literal, so the Anthropic request body sends a string where the API requires a JSON array, violating the Modeling Faithfulness and Explicit boundary contracts invariants.
| path: "/v1/messages", | ||
| body: { model: model, messages: messages, max_tokens: max_tokens, temperature: temperature, system: system } | ||
| body: { model: model, messages: messages, max_tokens: max_tokens, temperature: temperature, system: system }, | ||
| headers: { "anthropic-version": "2023-06-01" } |
There was a problem hiding this comment.
Invariant violation: The transport repeats anthropic-version and 2023-06-01 as header literals even though current_api_version already declares that fact, violating Single-authority metadata and MODELING.md M7.
| // fact in the transport definition (e.g., stdout from stdout, stderr from stderr, | ||
| // success from exit_code). Currently emits compile_error — model the | ||
| // channel→field mapping in std/ to dissolve this. | ||
| "compile_error!(\"shell multi-field output needs channel mapping in transport definition\")" |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Review (INVARIANTS: 3, MODELING: 2+/2-, ROADMAP: 0✓/1!) INVARIANTS — Violations (3) ROOT CAUSE ANALYSIS
MODELING — Strengths
MODELING — Improvements
ROADMAP — Incomplete
ROADMAP — Unclaimed
The PR makes real RE-4 progress and removes one prior shell-emission fabrication, but it still has a malformed Anthropic request body, a duplicated version authority, and a new emit-stage compile_error fallback. |
ChatGPT ReviewI reviewed the diff itself and treated My merge call: not ready as-is. I would block on two issues. 1. Invariant violationsBlocker:
|
briansrls
left a comment
briansrls
left a comment
There was a problem hiding this comment.
codex · gpt-5.4 · 61c2314d
Review (INVARIANTS: 3, MODELING: 3+/3-, ROADMAP: 0✓/2!)
INVARIANTS — Violations (3)
ROOT CAUSE ANALYSIS
dsl/extdeps/llm/anthropic.dagThe Anthropic boundary is still shaped around raw Strings and duplicated auth/model facts, so Layer 5 code must repeat provider ids and the service config competes with per-call inputs; make model selection a typed AnthropicModel/AnthropicModelSpec reference and let auth_source rewrite the callable signature so each fact has one structural authority end-to-end (M7, M9).src/v2/05_emit_rust.dagThe output projection carries cardinality on the field but emit_json_value_extract only models required extraction, so dry-run emission compensates downstream with a Some wrapper; introduce one structural projection/extractor that consumes path plus cardinality plus type and reuse it for both live and dry-run paths (M1, M8).
MODELING — Strengths
dsl/std/serialization.dagVariantEncoding and VariantNaming are a clean coproduct-based model of enum wire shape at the std layer, so provider modules can state wire facts without inventing provider-specific emitter logic.dsl/extdeps/github/github.dagRenaming HumanUser to User and attaching AsAuthored keeps the model faithful while staying at the provider-facts layer.dsl/extdeps/llm/llm.dagMoving role casing into a module-level wire contract keeps the shared LLM ontology provider-agnostic while still faithful to wire strings.
MODELING — Improvements
dsl/std/serialization.dagLand WireContract as the actual consumed authority and extend it to cover body format and field naming, otherwise emit must keep reconstructing wire facts from raw nodes instead of reading a closed contract.dsl/extdeps/llm/anthropic.dagPromote Messages.model and messages from String and Json to provider-grounded types so the Anthropic service composes from AnthropicModel and message algebras instead of string bridges.dsl/gunbc/tools/review.dagWhen transport body literals land, represent review_messages as typed message data and keep PR-specific context in the user-message channel rather than tunneling it through a JSON string at Layer 5.
ROADMAP — Incomplete
- RE-3: review.dag passes live integration: review_pr still computes already_done without branching on it and the Anthropic request payload still depends on the JSON-string message bridge, so the live workflow remains partial.
- RE-4: Anthropic REST API end-to-end: The PR integrates Anthropic REST, but model ids, credentials, and message bodies are still duplicated or bridged through Strings rather than flowing from structural authorities end-to-end.
ROADMAP — Unclaimed
- RE-4: Anthropic REST API end-to-end: This diff clearly advances RE-4 beyond Not started, but ROADMAP.md was not updated to reflect that the item is at least partial.
The PR fixes several earlier emitter regressions and materially advances RE-4, but it still adds new duplicate authorities and a cardinality leak, so Track 5 remains partial.
|
|
||
| data default_backend: String = "codex" | ||
| data default_model: String = "gpt-5.4" | ||
| data default_model: String = "claude-sonnet-4-6-20250929" |
There was a problem hiding this comment.
Invariant violation: default_model re-literals Anthropic's model id instead of reusing the provider authority already declared in anthropic.dag, violating Single-authority metadata and MODELING.md M7.
| endpoint: "https://api.anthropic.com" | ||
| auth: Header("x-api-key") | ||
| auth_input: api_key | ||
| auth_source: EnvVar { name: "ANTHROPIC_API_KEY" } |
There was a problem hiding this comment.
Invariant violation: auth_source adds env-based credential authority while Messages still exposes api_key as an operation input, violating Single-authority metadata and Explicit boundary contracts.
| let raw = match ch.inferred { | ||
| Some { value: Resolved { node: tn } } => | ||
| emit_json_value_extract(var_name: field_name, from_path: from_path, dag_type_name: tn.name) | ||
| _ => |
There was a problem hiding this comment.
Invariant violation: The new dry-run optional-field branch extracts the field as required and only then wraps it in Some, so absent optional fields still error instead of yielding None, violating Explicit boundary contracts and Emission is translation, not decision-making.
|
Review (INVARIANTS: 3, MODELING: 3+/3-, ROADMAP: 0✓/2!) INVARIANTS — Violations (3) ROOT CAUSE ANALYSIS
MODELING — Strengths
MODELING — Improvements
ROADMAP — Incomplete
ROADMAP — Unclaimed
The PR fixes several earlier emitter regressions and materially advances RE-4, but it still adds new duplicate authorities and a cardinality leak, so Track 5 remains partial. |
review.dag: Anthropic Messages REST API replaces CLI shell-out. Default model claude-sonnet-4-6. Prompt split into system + user message. BRIDGE: messages as JSON-encoded string (needs emit_simple_expr list literals). anthropic.dag: auth_source EnvVar, anthropic-version from current_api_version, service config endpoint. std/serialization.dag: VariantEncoding (StringVariant | TaggedVariant), VariantNaming (AsAuthored | SnakeCase), WireContract. Module-level wire_contract data declarations in github + llm extdeps. Emitter (05_emit_rust.dag): - Wire contract: per-enum serde tag from module wire_contract item - auth_token.clone() for Header auth with auth_source - Service config endpoint resolution - User-Agent on REST client - Serde rename for reserved keyword fields (ref, type) - Dry-run mock: multi-field tuple extraction + optional wrapping - Shell multi-field: JSON-parse bridge (needs channel mapping) BRIDGE items pending dissolution: - review_messages: string encoding of JSON array (emit_simple_expr) - Shell multi-field: stdout JSON parse (transport channel mapping) - Response format: all success responses use json() (needs WireFormat) - api_key param: still in operation signature with auth_source Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ChatGPT ReviewReviewed against the diff plus the attached invariants/modeling/roadmap docs. I’d block on 4 items.
chatgpt-review-c8ae25dc-0efe-4a…
Why I think this is a real invariant violation:
chatgpt-review-b2d42255-adc0-48…
Diff refs:
This is exactly the kind of “new boundary must land end-to-end” problem the invariants call out. chatgpt-review-b2d42255-adc0-48…
Diff refs:
Why I’d block on it:
chatgpt-review-b2d42255-adc0-48…
chatgpt-review-c8ae25dc-0efe-4a…
Diff refs:
This is a straightforward “no fallbacks that fabricate” issue. Dry-run is supposed to preserve shape, not force presence. chatgpt-review-b2d42255-adc0-48… Non-blocking notes
Roadmap
chatgpt-review-c8ae25dc-0efe-4a… Overall I would not approve as-is. The biggest blocker is the new module-level |
briansrls
left a comment
briansrls
left a comment
There was a problem hiding this comment.
codex · gpt-5.4 · 9ce7288e
Review (INVARIANTS: 3, MODELING: 3+/1-, ROADMAP: 0✓/1!)
INVARIANTS — Violations (3)
ROOT CAUSE ANALYSIS
src/v2/05_emit_rust.dagThe resolved output type already carries the upstream shape fact that distinguishes a named product from an anonymous conjunction, but this branch re-derives shape from connective plus arity and diverges from the same authority used by render_rust_type; factor one structural output-shape classifier from the resolved Node identity and make both dry-run and real response extraction consume it (MODELING.md M8).src/v2/05_emit_rust.dagService endpoint authority reaches emit as a real expression, but the service-constructor path only accepts raw strings, so non-literal endpoints are discarded and the legacy localhost fallback masks the loss downstream; move endpoint into a structural service-constructor or LanguageSpec input that can translate the declared expression end-to-end, and fail closed when that authority is missing (MODELING.md M8, Track 2).src/v2/05_emit_rust.dagThe emitter is mixing target-identifier normalization with wire-key authority: it already has exact authored_name and from_key facts, but re-derives keys from Rust naming rules because field-key serialization is not a single structural authority; use exact authored/from_key keys end-to-end, or extend std.serialization so field-key mapping is modeled once and consumed structurally by emit (MODELING.md M7-M8).
MODELING — Strengths
dsl/std/serialization.dagVariantEncoding and VariantNaming put wire-shape facts in a provider-agnostic std module instead of burying serde policy in Rust-specific emission logic.dsl/extdeps/llm/llm.dagRecasting Role as plain System, User, Assistant, and Tool under a shared snake_case wire contract is a faithful L2 model that composes cleanly into Anthropic and OpenAI.dsl/extdeps/github/github.dagRenaming HumanUser to User and declaring an AsAuthored string-enum contract matches GitHub’s actual wire values more faithfully at the provider-core layer.
MODELING — Improvements
dsl/std/serialization.dagWireContract should grow into the single structural authority for enum form, field keys, and response or body formats so Track 2 can delete the remaining rename and text-versus-json emitter heuristics.
ROADMAP — Incomplete
- RE-4: Anthropic REST API end-to-end: The diff starts the REST backend, but auth authority, request-body typing, and wire-format authority are still incomplete, so RE-4 is not done.
ROADMAP — Unclaimed
- RE-4: Anthropic REST API end-to-end: Because the diff clearly starts RE-4 with Anthropic provider wiring, review.dag integration, and RE-4 tests, ROADMAP should no longer leave this item at Not started.
The PR materially advances RE-4 and resolves several earlier emitter heuristics, but it still introduces new emit-side structure loss and leaves key Anthropic and wire-contract authorities incomplete.
| // serde_json::from_value can't deserialize JSON objects into tuples, | ||
| // so we extract fields individually and construct the tuple. | ||
| // Single-field conjunctions (return type = field type) use from_value. | ||
| let is_multi_field_conj = inferred.connective == Conj && inferred.children |> count > 1 |
There was a problem hiding this comment.
Invariant violation: emit_dry_run_branch_from_props treats every multi-field conjunction as tuple-shaped dry-run output, so named struct results are rebuilt as tuples, violating Explicit boundary contracts and Heuristics indicate lost structure.
| let base_url_default = if fs.has_rest { | ||
| // Find base_url: prefer service config endpoint, then transport base_url | ||
| let from_config = match service_config_endpoint(n: service_item, source_index: source_index) { | ||
| Some { value: ep } => match ep.expr_data { |
There was a problem hiding this comment.
Invariant violation: emit_service_new_method drops any non-literal config.endpoint expression to an empty string and then falls through to http://localhost, violating No fallbacks that fabricate and Emission is translation, not decision-making.
| // Reserved keywords: emit_ident adds r# prefix, but serde needs a rename | ||
| // to match the original JSON key (e.g., "ref" → r#ref needs #[serde(rename = "ref")]) | ||
| let authored = authored_name(env: env, node: child) | ||
| let snake = to_snake(name: authored) |
There was a problem hiding this comment.
Invariant violation: emit_struct_field_from_child derives reserved-field serde rename keys with to_snake(authored) instead of the declared wire key, so Rust keyword escaping can silently rewrite the JSON contract, violating Explicit boundary contracts and Emission is translation, not decision-making.
|
Review (INVARIANTS: 3, MODELING: 3+/1-, ROADMAP: 0✓/1!) INVARIANTS — Violations (3) ROOT CAUSE ANALYSIS
MODELING — Strengths
MODELING — Improvements
ROADMAP — Incomplete
ROADMAP — Unclaimed
The PR materially advances RE-4 and resolves several earlier emitter heuristics, but it still introduces new emit-side structure loss and leaves key Anthropic and wire-contract authorities incomplete. |
S1-S6 all done (were listed as "S4 in progress"). C1-C2 done. Track 6 done (AlgebraFieldKind coproduct, PR #387). RE-4 done (PR #384). Stream B Layer 2 done, Layer 1 reverted (blocked on Track 3). Track 7/10 items dissolved by PR #377/#387. Updated CI gates (390 tests, 421 violations), active branches table (quick-owl-889, PRs #388-390), clone count (23,733→13,820), and "what to start now" section to reflect C3-C6 as next unblocked work. https://claude.ai/code/session_01LtFw3MQ9qUq9mSwN7VLZMR
1 participant
Summary
anthropicwithclaude-sonnet-4-6.auth_source: EnvVar { name: "ANTHROPIC_API_KEY" },anthropic-versionheader, and service config endpoint.05_emit_rust.dag:self.auth_token.clone()for Header auth with auth_sourceSome()wrapping in mock extractionlocalhost)User-Agentheader on REST client (Client::builder().user_agent())#[serde(rename)]for reserved keyword fields (ref,type)response.text()for String response types (wasresponse.json())Live test status
Compiles, builds, dry-runs. Live execution reaches GitHub API (Get + ListReviews deserialize). Remaining: trace which later operation fails deserialization.
Test plan
review_dag_compiles_to_rust— 0 cargo check errorsreview_dag_builds_and_runs_dry_run— passes🤖 Generated with Claude Code