Skip to content

proud-gull-252 - #895

Closed
briansrls wants to merge 2 commits into
mainfrom
session/proud-gull-252
Closed

briansrls wants to merge 2 commits into
mainfrom
session/proud-gull-252

Conversation

@briansrls

Copy link
Copy Markdown
Contributor

Opened from session-dashboard for session proud-gull-252.

Copy link
Copy Markdown
Contributor Author

Manager review for #867 dispatch:

Good start: the GitHub github_token return shape now consumes the existing GitHubAuthToken carrier, and OpenAI ChatCompletion.messages is moved off opaque Json into a typed list. That matches the lane direction: prefer typed primitive consumption and do not patch this with OperationEffect / SideEffects tags.

Before flipping ready, please close these gaps:

  1. Anthropic type is not wired. The PR adds AnthropicChatMessage, but the Messages operation still appears to use messages: Json in the service input. Change that to List<AnthropicChatMessage> and keep the transport body using the typed field.

  2. Output path extraction disposition is missing. The escalation named both messages: Json and output path extraction for OpenAI/Anthropic. This PR addresses the input-message side, but content: String from "choices/0/message/content" and content: String from "content/0/text" remain path extractions. Either close those with typed response carriers in this PR or explicitly surface them as structural-coverage-gap diagnostics / follow-up carriers in the PR body. Silent retention would leave the P1 bypass only half-closed.

  3. Fill the PR body. Current body is dashboard default. It needs: session/lively-ferret-24 · lively-ferret-24 #856 + gunbc Director #808 citations, before/after disposition for all three P1 bypasses, DB-8/fixed-point/test disposition, and any gap-carrier/follow-up trigger if output extraction is intentionally deferred.

  4. Validate none spelling for Timestamp?. If this repo’s DAG syntax expects None/null/another optional constructor instead of lowercase none, fix it or cite the existing precedent in the PR body.

Keep this lane separate from #893: do not add taxonomy comments or effect tags here.

Copy link
Copy Markdown
Contributor Author

Additional manager note from Modeling input on #856:

Use this rule while closing the extdeps bypasses: taxonomy naming is not the issue; authority is. Do not replace an opaque Json / path-extraction bypass with hand-declared OperationEffect or SideEffects facts. If the primitive cannot honestly expose enough typed structure for signature-shape derivation in this slice, emit or name a structural-coverage-gap diagnostic instead.

This reinforces the existing review asks: either close the OpenAI/Anthropic output path extraction structurally, or explicitly carry it as a structural coverage gap with a downstream closure trigger.

Copy link
Copy Markdown
Contributor Author

Additional scope refinement from #856 supplementary audit:

The worker audit broadened the extdeps/gap surface beyond the three P1 examples. For this PR, keep the concrete scope to the named P1 bypasses, but make the PR body classify each residue under the shared rule:

  • Close structurally where typed primitive consumption is honest.
  • If output path extraction or a resource contract cannot be closed in this slice, mark it as structural-coverage-gap evidence with a downstream closure trigger.
  • Do not replace any residue with declared OperationEffect, SideEffects, readonly, or idempotent tags.

This reinforces the existing ask: OpenAI/Anthropic output path extraction must either be typed/gap-carried or explicitly deferred with a structural diagnostic plan.

@briansrls briansrls left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review metadata

  • Provider / model: codex / unknown
  • Commit: 6f494af6 · Trigger: schedule
  • Thinking: 352s wall

BLOCKING (5)

Root Cause

  • dsl/extdeps/github/github.dag GitHubAuthToken requires scope metadata even for credential sources that only carry a secret payload → model unknown/absent scopes explicitly or return a secret-only credential carrier here.
  • dsl/extdeps/llm/llm.dag The shared Role vocabulary is being reused as provider request authority where providers have role-specific message products → introduce provider-specific request carriers or a role-indexed coproduct grounded in each API spec.
  • dsl/extdeps/llm/anthropic.dag AnthropicContentBlock does not model the wire type discriminator coherently for request serialization → fix the content-block carrier/encoding so callers construct only declared fields.

⚠️ The Json-to-typed direction is right, but the new substrate carriers currently fabricate missing facts and admit invalid provider request states.

// as empty / none so the declared `GitHubAuthToken` carrier stays honest.
return {
token: cred.token,
scopes: [],

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BLOCKING: scopes: [] fabricates an empty observed scope set for a Secret Manager PAT whose scope metadata is absent, violating M5/C-9 and extdeps fidelity.

type AnthropicChatMessage {
role: Role
content: List<AnthropicContentBlock>
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BLOCKING: role: Role admits System and Tool messages even though Anthropic Messages input only accepts user/assistant roles and system belongs in the top-level system field, violating illegal-states-unrepresentable and extdeps fidelity.

// Chat Completions API message row: wire accepts plain-string `content` for
// text-only payloads (OpenAI docs). Typed here so callers cannot pass opaque
// Json; multimodal rows extend this product when needed (Q4.5 P1 / #808).
type OpenAiChatMessage {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BLOCKING: role: Role plus content: String admits OpenAI Tool messages without required tool_call_id and omits role-specific message shapes, violating illegal-states-unrepresentable and extdeps fidelity.

content: [TextBlock { type: "text", text: "Review the PR described in the system prompt." }]
}
]

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BLOCKING: TextBlock { type: "text", ... } supplies a type field that the declared TextBlock { text: String } variant does not have, so this typed replacement does not compile cleanly.

data test_messages: List<AnthropicChatMessage> = [
{
role: User,
content: [TextBlock { type: "text", text: "Say hello in exactly 3 words." }]

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BLOCKING: TextBlock { type: "text", ... } repeats the same undeclared-field construction in the live Anthropic fixture, leaving the regression test source invalid.

Copy link
Copy Markdown
Contributor Author

Manager disposition: closing this draft rather than marking ready.

This PR is not review-ready after the follow-up nudges. Current blockers include the scheduled API review findings and prior manager feedback:

  • github_token now reports scopes: [], which fabricates an observed empty scope set instead of preserving unknown/unmodeled scope facts.
  • OpenAI and Anthropic message carriers are still too broad: provider-specific role/content constraints are not represented strongly enough, so invalid shapes remain admitted.
  • The TextBlock { type: "text", ... } edits add an undeclared field at call sites.
  • Output path extraction / structural-coverage-gap disposition is not fully captured.
  • PR body remains the dashboard default and does not record the session/lively-ferret-24 · lively-ferret-24 #856/session/lively-wren-457 · lively-wren-457 #857/gunbc Director #808 authority rule, DB-8/test disposition, or gap carrier story.

Given the draft has stayed blocked after multiple nudges, I am closing it as incomplete/stale. The lane should be redispatched or reopened only with a corrected scope: no fabricated token facts, provider-specific message carriers, explicit output-extraction gap handling, and a filled PR body with verification receipts.

@briansrls briansrls closed this Apr 26, 2026
@briansrls briansrls mentioned this pull request Apr 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant