Repository navigation
test(firmware): exercise the staging ceiling through the emitted refusal - #7033
XiaoBinGan wants to merge 2 commits into
Conversation
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. |
 [Low impact] The test-only change appears safe to merge. SummaryAdds two tests that check the recovery advice emitted by
Tests were not run during this review. Supersession could not be checked because GitHub access required authentication. Reviews (8) · Last reviewed commit: "test(firmware): compare the emitted reco..." · Reviewed by Greptile |
|
Pushed The diff against Worth noting for the record: the flake had nothing to do with this PR's content, but a stale base still turns into a red PR that a reviewer has to spend time on. That is the same delivery-channel problem as #3442. |
![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: 60262d9dae
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| expect(projected.remediation.action).toContain("not supported"); | ||
| expect(projected.remediation.action).not.toContain("Increase"); | ||
| expect(projected.remediation.action).toContain("smaller firmware byte range"); |
There was a problem hiding this comment.
If firmwareStagingLimit is later replaced with an ad-hoc or truncated action that preserves these three snippets, this test still passes even though the emitted recovery has diverged from firmwareStagingBudgetRemediation, so it does not fully protect the wiring it was added for and duplicates brittle prose checks. Assert exact equality with firmwareStagingBudgetRemediation("max_output_bytes", MAX_FIRMWARE_STAGING_BYTES) here (and with 1 in the lower-budget case), while leaving recovery semantics to the formatter's module tests; this is the consumer/owner comparison required by the repository's test guidance.
AGENTS.md reference: AGENTS.md:L83-L83
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Adopted in 28a3ce4d1. Both cases now assert exact equality with
firmwareStagingBudgetRemediation(<field>, <selected value>) rather than matching
phrases, so a divergent emitted action fails even when it repeats the ceiling
sentences. Recovery semantics stay in the formatter's module test as you suggested.
Verified red: replacing firmwareStagingLimit's action with a sentence that keeps
"not supported" and "smaller firmware byte range" but diverges from the owner now
fails both cases, where the previous phrase matching passed.
Thanks — the point about not fully protecting the wiring while duplicating brittle
prose checks was exactly right.
18312f2 to
1b17d97
Compare
|
The failing checks here are not caused by this diff, which only adds one firmware test file. Three separate
Locally this PR's own test passes: The commit status for |
1b17d97 to
2768469
Compare
|
The Verified on a clean checkout of and For this PR specifically: it adds one test file under
|
2768469 to
1cdcebc
Compare
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
The ceiling recovery added in morluto#6971 was covered through the wording helper only, so a wiring change that passed the schema ceiling where the selected value belongs, or that dropped the remediation, would have stayed green. Adds boundary coverage that calls inventoryFirmwareOutput itself: a sparse file reaches the real byte ceiling without storing the bytes and asserts the projected refusal carries the ceiling recovery, and a one-byte tree below the ceiling asserts the raise advice. Verified red on both regressions — passing the schema ceiling for the selected value fails the below-ceiling case, and restoring the single-form remediation fails both.
The wiring test matched three phrases of the emitted remediation, so an ad-hoc or truncated action that kept those phrases would still pass while the emitted recovery had diverged from firmwareStagingBudgetRemediation. Assert exact equality with the owner's output for the selected value in both cases instead. Verified red: replacing the emitted action with a divergent sentence that keeps the ceiling phrases now fails both cases, where the phrase matching passed.
1cdcebc to
8fd1cf4
Compare
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
Closing to free a review slot, not because the work is wrong: this is test-only, so its The follow-up itself is unchanged and ready. Branch I will reopen it once |
Summary
#6971 established the staging-budget ceiling recovery but covered it through the wording helper
only. Review noted the follow-up: exercise the error
inventoryFirmwareOutputactually emits, so theproduction wiring is protected rather than just the string function.
This adds that boundary coverage. It is the follow-up requested in the #6971 review.
Problem and expected behavior
A wiring change that passed the schema ceiling where the selected value belongs, or that emitted an
ad-hoc action instead of the owner's, keeps a helper-only test green while the refusal handed to
callers is wrong. The gap is regression protection, not a behaviour defect: the current wiring is
correct.
Expected: the recovery emitted by
inventoryFirmwareOutputequalsfirmwareStagingBudgetRemediation(<field>, <selected value>)at the schema ceiling and below it.Change and scope
Adds
tests/boundary/firmware/firmwareStagingBudgetWiring.test.ts:MAX_FIRMWARE_STAGING_BYTES + 1. The file reports the size without storing the bytes, so the testexercises the real schema ceiling instead of a toy budget. The projected refusal must carry
reported_limits.max_output_bytesand must equal the owner's output exactly.the same exact-equality assertion covers the raise advice.
familiar sentences still fails.
Not included: no production change. The entry ceiling cannot be reached without staging more entries
than the schema permits, so its at-ceiling wording stays covered by
src/firmware/FirmwareStagingBudget.test.tsand its below-ceiling entry wiring byfirmwareReportFiles.test.ts. That limit is stated in the test file.Follow the contribution scope.
Unresolved scope, authority, side effects, or required verification block
integration. Link the required maintainer scope decision for new providers or
capabilities involving target execution, injection, mutation, or protection
recovery; ordinary fixes within existing boundaries may state "not applicable".
Draft PRs may leave the scope decision pending and the readiness and merge checklists
unchecked. Maintainers can help resolve open questions.
Not applicable — test-only coverage for an existing, already-integrated boundary.
Contract and boundary impact
changes.
FileHandle.truncate, which reports the declared length without storing it on the supported hosts;a host that allocated it would make the test slower, not wrong.
docs/public/product-catalog.json), package, or installation impact (CI generates and validates ignored catalogs andskills/; edit authored instructions in.agents/skills/reverse-engineer-anything/and never commit build outputs): none.Expected effects (check known effects; use documentation-only on its own).
Unchecked items do not establish absence; leave uncertainties visible.
Evidence and regression coverage
tests/boundary/firmware/firmwareStagingBudgetWiring.test.ts(2 cases).test catches. Passing the schema ceiling as the selected value makes the below-ceiling case fail.
Replacing the emitted action with a divergent sentence that keeps the ceiling phrases fails both
cases under exact equality, where phrase matching passed. Both checks were run and reverted.
than the schema permits, so only the byte ceiling is exercised end to end. The test asserts the
projected refusal, not a full
extract_firmwareMCP round trip, and no real Unblob extraction ran.For boundary fixes, identify the applicable boundary regressions
and lifecycle regressions
covered by the evidence above, and explain relevant gaps.
No boundary regression class applies: this changes no production code path, no reader or process
lifetime, and no public contract. It pins an existing refusal's recovery against its owner.
For evidence-bearing changes:
Validation performed
node node_modules/vitest/vitest.mjs run tests/boundary/firmware/firmwareStagingBudgetWiring.test.ts --maxWorkers=1— 2 passed.node node_modules/vitest/vitest.mjs run src/firmware tests/boundary/firmware --maxWorkers=1— 112passed, 52 skipped (13 files; the skips are host-gated firmware integration and workspace recovery).
npx tsc --noEmit -p .— no errors.npx oxfmt <changed file>— formatted.npm run verify:module-boundaries— 3012 source files checked, no violations.failed; divergent action keeping the phrases → 2 failed; restored → 2 passed.
Compatibility, safety, and release
Author readiness
Human owner: @XiaoBinGan
Before merge
type(scope): outcome.