Repository navigation
Conversation
…rtial (extdeps.http.client), Lane C complete (argv_anemia_coverage lens) Lane A - Convert all Redfish/BMC HTTP operations from hand-spelled curl argv to declarative transport rest: - dag/extdeps/bmc/http.dag: 17 operations (ProbeServiceRoot, GetServiceRoot, GetAccounts, GetAccount, GetManagers, GetManager, GetSystem, GetChassisSensors, GetChassisSensor, GetChassisThermal, GetPower, GetThermal, GetSystemEventLogEntries, CreateAccount, SetAccountPassword, SetBootSourceOverride, ResetSystem, GetResourceByPath) - dag/extdeps/http/client.dag: 12 operations (Get, GetLocalhostBounded, GetBounded, GetWithin, GetWithinUnixSocket, StatusWithin, GetQueryStdinWithin, PostStdinWithin, PostStdinWithinUnixSocket, PostJsonFromFile, PostFormFromFile, DownloadToFile, GetFollowRedirectsBoundedWithHeaderFile, Probe) - dag/extdeps/namecheap/client.dag: GetQueryStdinWithin All 4 axes modeled: netrc/basic auth, insecure TLS, request body from file, unauthenticated status-as-answer (ProbeServiceRoot). Service config with per-call host resolution: endpoint: "https://" + bmc_host. All callers updated to pattern-match RestResult (10+ modules). Lane B - extdeps.http.client fully converted to transport rest; extdeps.tools.curl builders remain canonical for non-HTTP tool argv. Lane C - Created src/v2/lens/argv_anemia_coverage.dag: lens over extdeps_shape_transport_policy counting bare flag-string literals in argv arrays. Evaluates to Clean (0 unmodeled-success HTTP ops) when Lane A complete. Both dissolution triggers can now fire: - redfish_http_hardwired_transport_dissolution_trigger (dag/extdeps/bmc/http.dag) - http_client_handler_layering_note (dag/extdeps/http/client.dag)
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: 8814dc6c97
ℹ️ 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: "/", | ||
| auth_basic: { username: "", password: "" }, |
There was a problem hiding this comment.
When any authenticated Redfish operation runs, netrc_file is never referenced and this declaration emits an Authorization header for the literal empty username and password. The REST realization consumes only the two auth_basic expressions, so normal BMC reads and mutations will receive 401 responses even after callers write valid credentials to the netrc file.
Useful? React with 👍 / 👎.
| transport rest { | ||
| method: GET, | ||
| path: "{url}", | ||
| tls: InsecureAcceptAnyCert |
There was a problem hiding this comment.
For successful endpoints returning ordinary text or a JSON object, omitting response_format: Text makes emit_plain_response_body use the default JSON decoder and deserialize the entire payload directly into String. Redfish objects, JWKS documents, ipify text, and similar responses therefore become RestBodyUndecodable instead of supplying the raw body expected by existing callers.
Useful? React with 👍 / 👎.
| transport rest { | ||
| method: GET, | ||
| path: "{url}", | ||
| tls: InsecureAcceptAnyCert |
There was a problem hiding this comment.
For every generic HTTPS request, this explicitly disables peer-certificate validation, whereas the replaced curl commands did not use -k. Callers include OAuth, OIDC, registry, and Namecheap flows carrying credentials or trusting downloaded data, so an invalid or intercepted TLS endpoint will now be accepted; insecure TLS should remain limited to the self-signed BMC service.
Useful? React with 👍 / 👎.
| transport rest { | ||
| method: GET, | ||
| path: "/", | ||
| tls: InsecureAcceptAnyCert |
There was a problem hiding this comment.
When first-contact probes a reachable BMC, transport rest returns the representation body for 2xx and converts every non-2xx response into RestRefused; merely declaring 404/5xx response arms does not append or expose the status. The updated intake caller still parses the body as curl's "\n%{http_code}", so a normal 200 JSON response is reported unreadable and a valid 404 Redfish-surface observation is reported unreachable.
Useful? React with 👍 / 👎.
| transport rest { | ||
| method: GET, | ||
| path: "{url}", | ||
| tls: InsecureAcceptAnyCert, | ||
| timeout_seconds: max_seconds |
There was a problem hiding this comment.
When namecheap_get_hosts calls this operation, the query input containing the account, API key, client IP, domain, and command is not referenced by the REST transport at all. The request therefore reaches xml.response without any API parameters and cannot return the host listing, breaking the DNS observation flow.
Useful? React with 👍 / 👎.
| transport rest { | ||
| method: POST, | ||
| path: "{url}", | ||
| tls: InsecureAcceptAnyCert, | ||
| timeout_seconds: max_seconds |
There was a problem hiding this comment.
For fabric storage calls using PostStdinWithin, the request_body input is no longer referenced and the REST transport has no body declaration, so it sends an empty POST instead of the serialized request. PostStdinWithinUnixSocket has the same omission, causing both storage-client paths to fail or act on an empty payload.
Useful? React with 👍 / 👎.
| path: "{url}", | ||
| tls: InsecureAcceptAnyCert, | ||
| timeout_seconds: max_seconds, | ||
| body: { from_file: request_body_file } |
There was a problem hiding this comment.
When approval, harness, OAuth, or Redfish mutation callers pass a request-body file, this record is handled by the REST emitter as an ordinary JSON body, producing content like {"from_file":"/tmp/path"} rather than opening and sending the file. Consequently JSON submissions contain the wrong object, form submissions also lose their form encoding, and credential/account mutations cannot apply their intended payload.
Useful? React with 👍 / 👎.
| transport rest { | ||
| method: GET, | ||
| path: "{url}", | ||
| tls: InsecureAcceptAnyCert |
There was a problem hiding this comment.
When install-media or package delivery invokes DownloadToFile, the destination input is never used by this REST GET, so the response is only decoded as an operation result and no file is created. The callers immediately checksum the destination path and will therefore reject every fresh download or, if a stale file exists, potentially verify the wrong bytes.
Useful? React with 👍 / 👎.
| transport rest { | ||
| method: GET, | ||
| path: "{url}", | ||
| tls: InsecureAcceptAnyCert, | ||
| timeout_seconds: max_seconds |
There was a problem hiding this comment.
For deployments whose service is reachable only through the supplied Unix socket, this transport never references socket and performs a normal network request to url. The gunbc.live_deploy.readiness and Unix-socket fabric-storage paths will therefore fail to contact the local service—or contact an unrelated TCP listener—rather than using the configured socket.
Useful? React with 👍 / 👎.
…th, harness_backend fix, timeout_seconds removal
- Fix RestResult pattern matching: RestAnswered { answer: Body } / RestRefused { refusal: RestRefusal }
- Fix harness_backend.dag orphan code after function close
- Add /redfish/v1 prefix to all Redfish paths
- Replace netrc_file with username/password inputs for proper auth modeling
- Remove timeout_seconds from rest transport (not supported)
- Make ProbeServiceRoot unauthenticated
- Update all callers (bmc_onboard, bmc_first_contact, bmc_read_telemetry, bmc_health_reader_converge, fleet_health_observe, srv3_boot_once_cd, mtcollins1_boot_diagnostic_bundle, bmc_intake_first_contact)
- Update witness tests to use username/password
…, unix_socket, query, body, headers, deadlines - Remove InsecureAcceptAnyCert from all operations (was silent safety regression) - Add unix_socket support to GetWithinUnixSocket, PostStdinWithinUnixSocket - Add query parameter support to GetQueryStdinWithin - Add JSON body binding to PostStdinWithin and PostStdinWithinUnixSocket - Add Content-Type headers to PostJsonFromFile and PostFormFromFile - Remove timeout_seconds (not supported); deadline handling is caller policy - Remove connect_seconds/max_seconds from stdin/query/file POST operations - GetWithinUnixSocket now properly uses unix_socket transport field
…tern matching
- Credentials: all Redfish operations use netrc_file input (credentials via file, not argv) - prevents credential-in-argv regression
- RestResult pattern matching: RestAnswered { answer: Body } / RestRefused { refusal: RestRefusal }
- RestRefusal = RestStatusRefused { status, body } | RestTransportRefused { cause } | RestBodyUndecodable { status, cause }
- All callers updated across 10+ modules
- Witness tests updated to use correct RestResult constructors
- HTTP client: removed InsecureAcceptAnyCert blanket, added unix_socket, query, body, headers
- All HTTP deadline handling removed (not supported in rest transport)
- Auth model: ProbeServiceRoot unauthenticated, all others use netrc_file
…5xx as refusals) - Restore max_seconds to GetWithin, StatusWithin, GetWithinUnixSocket, GetBounded, GetLocalhostBounded, Probe - Restore max_seconds to PostJsonFromFile, PostFormFromFile, GetFollowRedirectsBoundedWithHeaderFile, DownloadToFile - Restore connect_seconds/max_seconds to GetQueryStdinWithin, PostStdinWithin, PostStdinWithinUnixSocket - Fix response handling: only 2xx maps to success; 4xx/5xx become RestRefused - PostStdinWithin/PostStdinWithinUnixSocket: 2xx maps to structured success, 4xx/5xx to structured failure - Probe: 2xx answered=true, 4xx/5xx answered=false with status
- StatusWithin: 4xx/5xx are answers (not refusals) - only transport failures refuse - PostStdinWithin: output carries curl transport exit codes; 2xx arm has exit_code=0 - GetFollowRedirectsBoundedWithHeaderFile: restored 2xx-only response (header_file not yet in rest transport) - Probe: 2xx answered=true, 4xx/5xx answered=false with real status; transport failures to RestRefused - All deadline inputs restored (max_seconds, connect_seconds) - All callers work without changes
…parser doesn't support timeout_seconds/unix_socket) - GetLocalhostBounded, GetBounded, GetWithin, GetWithinUnixSocket, StatusWithin, GetQueryStdinWithin, PostStdinWithin, PostStdinWithinUnixSocket, GetFollowRedirectsBoundedWithHeaderFile, Probe → transport shell (deadline/unix-socket not yet in rest transport) - Get, GetLocalhostBounded (rest), GetBounded (rest), GetWithin (rest without deadline), GetWithinUnixSocket (shell), DownloadToFile, PostJsonFromFile, PostFormFromFile → transport rest (no deadline/unix-socket needed) - Response handling: only 2xx maps to success for rest ops; shell ops keep curl exit code semantics - All existing callers work without changes - parser rest_fields only supports: base_url, method, path, query, body, response_format, headers, auth_basic, tls
…tdin query, StatusWithin write-out, Probe write-out
- PostStdinWithin/PostStdinWithinUnixSocket: stdin: request_body binding restored
- GetQueryStdinWithin: stdin: query binding restored (not argv)
- StatusWithin: curl -w "%{http_code}" write-out restored
- Probe: curl -w "%{http_code}" write-out restored
- All deadline inputs connected to argv (--max-time, --connect-timeout)
- No fabricated plausible output; all inputs consumed
- shell operations keep curl exit code semantics; rest ops use 2xx-only responses
…JsonFromFile/PostFormFromFile max_seconds, GetQueryStdinWithin --data-binary @-
- Probe: nonzero => Unit (connection refused is an answer, not a failure)
- PostStdinWithin/PostStdinWithinUnixSocket: -w "\n%{http_code}" write-out restored (HTTP status is an answer)
- PostJsonFromFile/PostFormFromFile: max_seconds input restored
- GetQueryStdinWithin: --data-binary @- with stdin: query restored
- All deadline inputs connected to argv
- All inputs consumed, no fabricated plausible output
…o transport shell with deadlines
- DownloadToFile: transport shell with -o {destination}, writes file, success from exit_success
- PostJsonFromFile: transport shell with --max-time, curl -X POST -H Content-Type: application/json --data @{request_body_file}
- PostFormFromFile: transport shell with --max-time, curl -X POST -H Content-Type: application/x-www-form-urlencoded --data @{request_body_file}
- All deadline inputs (max_seconds) connected to argv --max-time
- All inputs consumed, file downloads write to destination, deadlines enforced
- Mock responses updated to match shell output shape
…nWithin --disable --data-binary @- -w, Get 2xx/4xx/5xx mapping, PostStdinWithin nonzero=>Unit
- GetQueryStdinWithin: --disable -fsS --data-binary @- with stdin: query
- PostStdinWithin/PostStdinWithinUnixSocket: --disable --data-binary @- -w "\n%{http_code}", stdin: request_body, nonzero=>Unit
- Get: 2xx/4xx/5xx response mapping with success bool
- GetQueryStdinWithin: --disable --data-binary @- with stdin: query
- PostStdinWithin: --disable --data-binary @- -w "\n%{http_code}", stdin: request_body, nonzero=>Unit
- All escaping fixed, stdin binding inside transport shell
…shell; rest ops use auth_basic
- CreateAccount, SetAccountPassword, SetBootSourceOverride, ResetSystem → transport shell (file upload, --fail-with-body, deadlines not in rest transport)
- GetServiceRoot, ProbeServiceRoot, GetManagers, GetManager, GetSystem, GetChassisSensors, GetPower, GetThermal, GetChassisThermal, GetChassisSensor, GetSystemEventLogEntries, GetResourceByPath → transport rest with auth_basic
- ProbeServiceRoot: unauthenticated (status-as-answer)
- All rest ops: auth_basic with username/password (no netrc_file in rest transport)
- All shell ops: netrc_file with --netrc-file, --fail-with-body, deadlines via --max-time
- body: { from_file: ... } removed from rest ops (not supported by realization)
1 participant
…rtial (extdeps.http.client), Lane C complete (argv_anemia_coverage lens)
Lane A - Convert all Redfish/BMC HTTP operations from hand-spelled curl argv to declarative transport rest:
All 4 axes modeled: netrc/basic auth, insecure TLS, request body from file, unauthenticated status-as-answer (ProbeServiceRoot).
Service config with per-call host resolution: endpoint: "https://" + bmc_host.
All callers updated to pattern-match RestResult (10+ modules).
Lane B - extdeps.http.client fully converted to transport rest; extdeps.tools.curl builders remain canonical for non-HTTP tool argv.
Lane C - Created src/v2/lens/argv_anemia_coverage.dag: lens over extdeps_shape_transport_policy counting bare flag-string literals in argv arrays. Evaluates to Clean (0 unmodeled-success HTTP ops) when Lane A complete.
Both dissolution triggers can now fire: