feat(#6778): add fullsend inference openai command family - #6779
Conversation
Add three new subcommands under `fullsend inference openai` for OpenAI Workload Identity Federation enrollment: - `request <owner/repo>[,...]`: generates the provider/mapping request document an admin needs, with every value computed from the repository names. Supports --format json (versioned schema for future API submission) and --format md (copy-paste ticket). - `import [reply.json]`: takes the admin's reply and writes inference.openai into .fullsend/config.yaml via the existing SetInferenceOpenAI setter, or sets FULLSEND_OPENAI_* repository variables with --variables. Enforces the all-three-or-none rule. - `status <owner/repo>`: prints resolved identifiers and their source (config.yaml or env vars), flags a partial trio, and when inside GitHub Actions performs one exchange reporting scope and expiry without printing the token. The commands need no credentials and do not call the OpenAI API. The JSON request schema is versioned so a future `apply` command can submit it unchanged when OpenAI exposes an API for providers. Updates docs/cli/inference.md with the new command reference and adds route-B shortcut pointers in the OpenAI WIF guide. Closes #6778
|
/fs-fix-stop |
|
Fix agent disabled for this PR. Remove the |
Supersedes #6777, which rewrote the operator guide's setup half while this branch was being written. The guide read as if managing the organization's identity provider were a prerequisite; in most companies an IT administrator owns Organization Settings and the GitHub Actions provider, and a team owns (or requests) a project. It is now an explicit fork: shared claims check first, then route A (you manage providers) or route B (send the administrator a request), with common steps from "tell fullsend the three identifiers" on. Trust is stated as per repository for both routes: one mapping per company-owned repository, never a wildcard or a pattern over the organization, since an organization can contain repositories the company does not own. Mappings match exact values only and have no list form. The new commands are wired into that structure rather than bolted onto the old sections: route A points at `request` for the console fields, route B generates its ticket with `request --format md` (the hand-written template stays for readers who prefer it), B3 records the reply with `import` and checks it with `status`, and step 4 keeps a shortcut note. Also fixes the pi runtime doc's "three repository variables" wording. Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
|
🤖 Review · Commit: |
Site previewPreview: https://40f4728f-site.fullsend-ai.workers.dev Commit: |
|
🤖 Review · Commit: |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
0cf11b5 to
e4d22cc
Compare
|
🤖 Review · Commit: |
Review findings from Codex (gpt-5.6-sol) and Grok 4.6, which agreed on the two blocking ones. import could not read the document request generates. The request JSON nests the fields an administrator fills under "reply", while import unmarshalled them from the root, so returning the very file we sent imported nothing. It now accepts both shapes, and picks the service account for --repo out of a multi-repository reply instead of silently importing none — an ambiguous reply says which repositories it names. status resolved each identifier independently, env or config per field, while a run takes the trio from one source: any FULLSEND_OPENAI_* variable means all three come from variables. status could therefore report a healthy trio, and exchange on it, that a run would refuse. It now applies the same rule, names the source, and explains the gap. Also in status: a malformed config is an error rather than "nothing configured yet"; an exchange is refused when the job's own repository is not the one being checked, since it could only ever prove that job's identity; and a token broader than model access fails the way the run path fails instead of printing "Exchange succeeded". In request: --ref for repositories whose default branch is not main (the assertion is exact, so a wrong ref fails every exchange), a guard against deriving one owner's default audience for another owner's mapping, duplicate repositories collapsed, an existing --service-account no longer described as "create inline", the permission list rendered rather than indexed, and the generated rules now name `sub` too. import validates the config before writing it, as `github setup` does, and a partial --variables write says which variables were set. Docs: the inference command group is no longer described as GCP-only, the "these commands do not call the OpenAI API" claim is qualified for status, and the command tree and key-files table in cli-internals cover the new subcommands. Assisted-by: Claude (fix), Codex (review), Grok (review) Signed-off-by: Wayne Sun <gsun@redhat.com>
eaaa663 to
14c970b
Compare
|
🤖 Finished Review · ✅ Success · Started 4:40 PM UTC · Completed 5:00 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $10.93 |
|
Risk Assessment: moderate (2/5) DetailsRe-review anchored to prior score. Large but mostly additive feature (new files + docs) from a bot author with good test coverage; elevated line count is offset by additive-only changes and clear acceptance criteria alignment. Only bug-fix changes since prior review; Tier 1 signals unchanged. Previous runRisk Assessment: moderate (2/5) DetailsLarge but mostly additive feature (new files + docs) from a bot author with good test coverage; elevated line count is offset by additive-only changes and clear acceptance criteria alignment. |
ReviewFindingsMedium
Next steps:
Previous runReviewFindingsMedium
Low
Next steps:
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Second review pass by Codex (gpt-5.6-sol) against the fixes, which confirmed five items closed and found four still open. status announced "Exchange succeeded" and printed the granted scope before checking it, so a token the run path refuses was reported as a success and only then rejected. The scope is checked first now, and nothing is called a success until it passes. The repository guard failed open: with a valid OIDC endpoint but no GITHUB_REPOSITORY, the mismatch check was skipped and the exchange ran, attributing the result to whatever repository was on the command line. An unknown job repository is now refused like a mismatched one. status still diverged from the run path in one case: run ignores the committed inference.openai block when there is no OIDC endpoint and OPENAI_API_KEY is set (a developer's key is not overridden by the repository's CI configuration), while status read the block anyway and named it as the source. It now applies the same rule and says so. import applied --service-account-id only after the reply document had already failed on the ambiguity that flag exists to resolve; the flag is now honoured first. Repository keys — deduplication, the mixed-owner guard, and reply lookup — compare case-insensitively, as GitHub does. Docs: the network claim is now a per-command table (request is offline, import reaches GitHub only with --variables, status exchanges with OpenAI inside Actions), --ref is documented, --repo's two roles are spelled out, and the schema comment no longer promises compatibility with an API OpenAI does not offer. The guide now states the mapping's real trust boundary — any job in the repository at the asserted ref that can request an OIDC token, not one named workflow — so a reader weighing "is it safe to commit these identifiers" sees that write access to main is the boundary that matters, and the spend limit is what bounds it. Tests: the scope path asserts the output carries no success line, the unknown-repository and static-key cases are pinned, and every status test now fixes OPENAI_API_KEY as well — one of them passed here only because of a key in the author's own shell. Assisted-by: Claude (fix), Codex (review), Grok (review) Signed-off-by: Wayne Sun <gsun@redhat.com>
|
🤖 Review · Commit: |
Third review pass by Grok 4.6, whose blocking finding was an architectural one: `import --variables` shelled out to the gh CLI. docs/contributing/forge-abstraction.md prohibits that outside internal/forge/github, and forge.Client.CreateOrUpdateRepoVariable is what `github setup` and `github set` already use. It now goes through the same client and the same token resolution, so it needs no gh binary on PATH and reports API errors like every other forge call; the partial-write warning is kept, and a test fails the second write to prove it. Correctness, from the same pass: - A full request document could lose the audience. An administrator reusing an existing provider is told to record its audience in the provider block, but import read only the reply section, so it kept the audience we had proposed and every exchange then failed the assertion. The provider block is now the fallback. - status exited 0 for an unconfigured repository and for a partial trio — the two states a run refuses — so it could not gate a check the way the GCP status command does. Both now exit non-zero. - status met an org-mode config with a generic "nothing configured"; it now names it as import does (org mode is deprecated, ADR 0044). - A reply naming several repositories with only one service account filled in was treated as unambiguous; filling the second in later would then have silently misattributed it. A selector is required whenever the reply names more than one repository. - request now trims its flag values, so a whitespace-only --audience cannot slip past the mixed-owner guard into the document. Docs: the inference group is described accurately in docs/cli/README.md and the operations command index; --service-account's default column no longer claims a value the flag does not have; the guide and the CLI reference both say import writes locally and the file must be committed; and the schema's long help matches the version constant's comment. Assisted-by: Claude (fix), Codex (review), Grok (review) Signed-off-by: Wayne Sun <gsun@redhat.com>
|
🤖 Finished Review · ✅ Success · Started 5:26 PM UTC · Completed 5:42 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $6.91 |
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Approving after three review rounds (Codex on gpt-5.6-sol, Grok 4.6, plus fullsend's own review agent), with every finding either fixed or answered on the thread.
Round 3's blocking finding was architectural and is fixed, not merged over: import --variables shelled out to the gh CLI, which docs/contributing/forge-abstraction.md prohibits outside internal/forge/github. It now uses forge.Client.CreateOrUpdateRepoVariable and the standard token resolution, like github setup; the partial-write warning is kept and covered by a test that fails the second write.
Also fixed in this round: a full request document could lose the audience when an administrator recorded it in the provider block (the shape the guide asks for); status exited 0 for an unconfigured repository and for a partial trio — the two states a run refuses — so it could not gate a check the way the GCP status does; an org-mode config is now named rather than reported as "nothing configured"; and a reply naming several repositories with only one service account filled in now requires a selector instead of being auto-picked.
Earlier rounds closed the round-trip bug (the document request generates is now the document import reads), aligned status's source resolution with resolveOpenAICredential including the static-key case, made the GITHUB_REPOSITORY guard fail closed, checked the token scope before anything is called a success, added --ref, and made repository keys case-insensitive.
The guide now states the mapping's real trust boundary — any job in the repository at the asserted ref that can request an OIDC token, not one named workflow — so a reader weighing "is it safe to commit these identifiers" sees that write access to main is what bounds it.
CI green on b8a47cc; all review threads resolved. Supersedes #6777.
|
🤖 Finished Retro · ✅ Success · Started 6:00 PM UTC · Completed 6:15 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $5.71 |
Retro: PR #6779 —
|
Codex (gpt-5.6-sol) and Grok 4.6 reviewed the branch; these are the findings both or either confirmed. Move the mapping-scope decision out of ADR 0092 into ADR 0095. 0092 is Accepted, and CONTRIBUTING says an accepted ADR takes cross-references, not new decisions. 0092 never documented the ref assertion, so this is a new decision rather than a rewrite of its own; 0092 keeps a pointer. The new ADR also states the boundary plainly: fullsend's dispatch authorization governs which runs fullsend starts and does not stand between a repository workflow and OpenAI's token endpoint. Refuse an import document whose provider block and reply disagree about the audience. `request` pre-fills reply.audience, so the provider-block fallback added in #6779 could never fire, and an administrator who reused a provider and edited only the provider block had that edit silently discarded -- recording an audience no mapping asserts, whose every exchange then fails with a 4xx far from the cause. --audience stays the way past it. Render one reply row per repository rather than one per mapping: with --ref the table asked twice for the same repository's service account, which reads as two accounts. Validate --ref as a full ref, and skip the companion mapping when the value already is refs/pull/*, which produced two identical mappings. Warn when a document exceeds OpenAI's 50 mappings per provider, since --ref doubles the count and the excess cannot be installed as written. Correct three doc claims: cli-internals still documented the old refs/heads/main default; the guide said only someone who can merge to the default branch can obtain a token, when a same-repository pull_request run executes a workflow from the pushed branch; and the help text and CLI reference said `import --variables` calls GitHub through gh when it goes through the forge client. Assisted-by: Claude (fix), Codex (review), Grok (review) Signed-off-by: Wayne Sun <gsun@redhat.com>
…md bullet Add a top-level bullet point in the "How to work in this repo" section for the forge-abstraction rule, placed after the Go coverage gate and before the secrets bullet. The rule was previously discoverable only via the topic-guidance table, which required agents to recognize that setting repository variables is a forge operation — a connection the code agent on PR fullsend-ai#6779 failed to make. Promoting it to an inline bullet mirrors the pattern used for other critical rules (DCO, coverage gate, CLI docs) and makes the architectural invariant harder to miss for both code and review agents. Closes fullsend-ai#6781
1 participant
Add three new subcommands under
fullsend inference openaifor OpenAI Workload Identity Federation enrollment:request <owner/repo>[,...]: generates the provider/mappingrequest document an admin needs, with every value computed from
the repository names. Supports --format json (versioned schema
for future API submission) and --format md (copy-paste ticket).
import [reply.json]: takes the admin's reply and writesinference.openai into .fullsend/config.yaml via the existing
SetInferenceOpenAI setter, or sets FULLSEND_OPENAI_* repository
variables with --variables. Enforces the all-three-or-none rule.
status <owner/repo>: prints resolved identifiers and theirsource (config.yaml or env vars), flags a partial trio, and
when inside GitHub Actions performs one exchange reporting scope
and expiry without printing the token.
The commands need no credentials and do not call the OpenAI API. The JSON request schema is versioned so a future
applycommand can submit it unchanged when OpenAI exposes an API for providers.Updates docs/cli/inference.md with the new command reference and adds route-B shortcut pointers in the OpenAI WIF guide.
Closes #6778
Post-script verification
agent/6778-inference-openai-commands)35775003b1b177088a3eb6610111986358ed1322..HEAD)Also folds in #6777 (docs)
PR #6777 rewrote the setup half of
docs/guides/infrastructure/openai-workload-identity.mdwhile this branch was being written; triage flagged the coordination. It is folded in here and #6777 is closed, because the guide change and these commands are the same piece of work — the commands exist to remove the hand-copying the guide used to describe.repository_owner, a wildcard, a prefix or a derived attribute, because a GitHub organization can contain repositories the company does not own. OpenAI mappings are AND-ed exact-value rows and have no list form, so this is also the only shape the console supports.requestfor the console fields, route B generates its ticket withrequest --format md(the hand-written template stays for readers who prefer it), B3 records the reply withimportand checks it withstatus, and step 4 keeps a shortcut note.docs/runtimes/pi.md's "three repository variables" wording is fixed.Closes #6777.