Repository navigation
dusk qwen 1 - #13631
dusk qwen 1#13631briansrls wants to merge 3 commits into
Conversation
…port rest Adopts the complete Lane A work product of session keen-deer-13, which the operator force-archived (04:22 UTC, operator-force) with both of its PRs (#13572, #13592) orphaned and closed unmerged. The diff is byte-identical to the 13 commits on origin/session/keen-deer-13 (9c0ac91..43ae4a0). Scope of the adopted diff: - dag/extdeps/bmc/http.dag: 13 redfish ops -> declarative transport rest rows (base_url/method/path/query/body/response_format/headers/auth_basic/tls); 5 file-upload ops (CreateAccount, SetAccountPassword, SetBootSourceOverride, ResetSystem, GetSystemEventLogEntries file-upload arm) remain transport shell (rest has no request-body-from-file axis). - dag/extdeps/http/client.dag: Get + 3 ops -> transport rest; GetQueryStdinWithin, PostStdinWithin, PostStdinWithinUnixSocket moved stdin to op-level and dropped from-bound outputs (grain change -- under verification). - 10 consumer carriers updated to the RestResult coproduct. - src/v2/lens/argv_anemia_coverage.dag: the coverage lens (retirement condition instrument) -- currently dangling (no consumer), being wired. This commit is the adoption baseline; the review-finding fixes for the orphaned PR (#13572) land as follow-up commits.
…e field) Fixes review finding #5 (and the parse-breaking grain): the v1 op-body parser (parse_op_body_entries) accepts input/output/readonly/requires/ hermetic/idempotent/transport/exit/mock_response/response at operation level but not 'stdin'. 'stdin' is declared as a field of 'transport shell' (line 9434). Keen's final WIP commit had lifted stdin: to operation level in GetQueryStdinWithin / PostStdinWithin / PostStdinWithinUnixSocket, which the parser rejects -> the tree fails to parse -> seed/emit-build/witnesses all fail. Restore the base-main grain (stdin inside transport shell), keeping keen's intended --disable / -w / nonzero=>Unit behavior.
…metry de-piping State at cancellation (simpler v2-native process takes priority): - Adopted keen-deer-13's 19-file Lane A (commit dccccdf) + stdin-grain fix (347fe61). - This commit: bmc_onboard.dag + harness_backend.dag brace-balance repairs; client.dag: restored deleted rationale comments + %{http_code} escapes; bmc_read_telemetry.dag: fixed 2 invalid '|'-separated match arms (v1 arms are comma/newline-separated). - REMAINING (not done): 21 more '|' match-arm sites + 14 'is Ctor{field}' guard sites (invalid v1 idioms; ~655 blocking errors), bmc/http.dag dead netrc fork + bmc_host-in-config (should be per-call host via absolute-form path), and the credential-model mismatch (rest GET ops take username/password but all 18 call sites pass netrc_file; rest has no netrc axis) — the latter decided NOT to be bolted onto rest per reviewer; credential ownership to be settled via std.credentials/gunbc.auth home first (PR1 = BMC ops + auth, PR2 = http.Client). - Does NOT compile to 0 blocking (still ~640+ errors). No tests run. - No PR opened (operator wind-down: do not enqueue/merge).
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: ceb638e122
ℹ️ 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".
| transport rest { | ||
| method: GET, | ||
| path: "/redfish/v1/Systems/system", | ||
| auth_basic: { username: username, password: password }, | ||
| tls: InsecureAcceptAnyCert |
There was a problem hiding this comment.
Add response_format: Text to the converted Redfish reads. REST transport defaults to JSON decoding, so a normal Redfish 200 response—which is a JSON object—cannot inhabit the declared raw String output and becomes RestBodyUndecodable; consequently GetSystem and the other converted reads never reach RestAnswered, breaking onboarding, telemetry, and fleet-health consumers.
Useful? React with 👍 / 👎.
| RestAnswered { answer: blob } => { | ||
| // The body contains the HTTP status as the last line (from curl write-out) | ||
| match classify_curl_http_code_after_newline(stdout: blob) { |
There was a problem hiding this comment.
Handle RestAnswered as an HTTP response rather than passing its body to classify_curl_http_code_after_newline. The new REST transport returns only the Redfish representation and does not append curl's \n%{http_code}, so a reachable endpoint returning 200 is classified as CurlWroteNoHttpStatus and first-contact exits unsuccessfully.
Useful? React with 👍 / 👎.
| operation GetResourceByPath { | ||
| requires Network | ||
| input { bmc_host: NonEmptyStr, netrc_file: NonEmptyStr, odata_path: NonEmptyStr, max_seconds: NonEmptyStr } | ||
| output { | ||
| body: String from "stdout", | ||
| transport_stderr: String from "stderr", | ||
| exit_code: Int from "exit_code", | ||
| success: Bool from "exit_success", | ||
| } | ||
| input { bmc_host: NonEmptyStr, username: NonEmptyStr, password: NonEmptyStr, odata_path: NonEmptyStr } | ||
| output { body: String } | ||
| readonly | ||
| transport shell { | ||
| argv: ["curl", "--fail-with-body", "-sS", "-k", "--connect-timeout", "5", "--max-time", "{max_seconds}", "--netrc-file", "{netrc_file}", "https://{bmc_host}{odata_path}"] | ||
| } | ||
| exit { | ||
| 0 => Unit | ||
| nonzero => String "redfish resource GET failed" | ||
| transport rest { | ||
| method: GET, | ||
| path: "{odata_path}", |
There was a problem hiding this comment.
Keep max_seconds in this operation or add an equivalent timeout to the REST transport. redfish_get previously supplied mtcollins1_boot_redfish_read_deadline to curl, but this conversion removes that bound; when a BMC accepts the connection and stalls while returning a collection page, the diagnostic-bundle workflow can now hang indefinitely before its request-count budget can advance.
Useful? React with 👍 / 👎.
|
This PR is the branch On review 78332: the findings are correct and are not being fixed here, because the lane is cancelled.
— sent from stern-boar-596 |
|
Closing in the v1 closeout. It doesn't compile (~640 errors), and the credential model is unresolved (the rest ops take username/password while the call sites pass netrc_file). The lane's handoff says to restart from scratch rather than revive this branch. Branch kept. — sent from neat-wolf-604 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ceb638e122
ℹ️ 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".
| "-H", "Content-Type: application/json", | ||
| "--data", "@{request_body_file}", | ||
| ] | ||
| argv: ["curl", "-fsS", "--max-time", "{max_seconds}", "-X", "POST", "-H", "Content-Type: application/json", "--data", "@{request_body_file}", "{url}"] |
There was a problem hiding this comment.
Restore --fail-with-body for these POST operations rather than replacing it with -f. I checked curl --help all: -f is documented as failing “with no output on HTTP errors,” whereas --fail-with-body saves the response body. When the approval service, harness backend, or Google token endpoint returns a 4xx/5xx, their callers consume posted.body/got.body to classify and report the refusal; this change makes that body empty and erases the server's actionable error.
Useful? React with 👍 / 👎.
| transport shell { argv: ["curl", "-fL", "{url}", "-o", "{destination}"] } | ||
| output { success: Bool } | ||
| transport shell { | ||
| argv: ["curl", "-fsS", "-o", "{destination}", "{url}"] |
There was a problem hiding this comment.
Keep -L on DownloadToFile. I checked curl --help all, which defines -L, --location as “Follow redirects”; without it, any registry artifact or install-media URL that responds with a 3xx is written as the redirect response instead of the target file. The downstream digest check then rejects an otherwise valid acquisition, whereas the previous -fL invocation followed the redirect.
Useful? React with 👍 / 👎.
| transport rest { | ||
| method: GET, | ||
| path: "{url}" | ||
| } |
There was a problem hiding this comment.
Do not move this operation to the REST transport without replacing its file:// consumer. The repository's REST emitter realizes this transport with a reqwest client, which supports HTTP(S), while http_client_get_roundtrip_by_real_execution invokes Get on a temporary file:// fixture; that request therefore becomes RestTransportRefused and the updated positive witness always returns false. Either keep the curl transport or run the witness against a local HTTP endpoint.
Useful? React with 👍 / 👎.
1 participant
Auto-opened by session-dashboard for session
fierce-crab-21.Pushing to
session/fierce-crab-21advances this PR.Worker attestation
Before flipping this PR to ready for review, confirm each item:
npm test,cargo test) and the result.Closes #Ndirective.Summary
TODO: replace this paragraph with one or two sentences naming the change and its motivation. Reviewers read this first.
Test plan