Repository navigation
Flag runner-refused credential store lines - #849
Merged
seathatflowsinourveins merged 8 commits intoOct 8, 2026
Merged
seathatflowsinourveins merged 8 commits into
seathatflowsinourveins merged 8 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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0ca21c31b3
ℹ️ 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".
seathatflowsinourveins
deleted the
codex/ns2604-credential-status-grammar-20261008
branch
October 8, 2026 18:07
6 of 8 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Scope
The credential status command reported a private store as
okeven when the key runner refused its first line. It now reports every grammar refusal asline N: reason_codeand marks the entry unsafe, while valid export-form stores remainok.618dd6c05ff6750705b52426261748d11e003477, merged into this branch at772df26ea7a793e2ffc36afbc4a65b6b47739718before final affected-module verification.7a3637f5df7111fda87f2f1408e23963d18caadc.lane:foundation.The runner's existing per-line body is extracted into one
parse_linefunction used by both tools. The runner still stops at its first refusal; status collects all refused lines. Status uses the runner's exactinjectable()predicate, invokes its core-dump guard before the checked file-descriptor reader, and reports completed grammar reads withvalues_read. Raw private files, native sign-in, service-held and undeclared files retain their metadata-only handling. Text output discloses the checked grammar read. The boot recorder explicitly selects its existing metadata-only scan.The shared reader's lazy import also exposes nine pre-existing runner/setter read sites to the OpenHands advisory import closure. Its committed monitoring baseline follows this PR's native report (330 reads/97 shape locations); classification, enforcement, breadth limits, workflow and assertions are unchanged. Main's independent 321-read baseline remains appropriate for main's source.
SOTA sources
LINEpattern,parseat lines252-285, and writer-grammar checks. The extracted line body has the same AST as that body except the former comment/blank-linecontinuebecomes helperreturn; no grammar is reimplemented in status.BARE_VALUEandencodechecks.osreference: the existing reader usesdir_fd,O_NOFOLLOW,fstat, boundedread, and descriptor cleanup. This PR reuses that reader.injectable()at line151 selects only runner-eligible private env files;check_core_pattern()anddisable_core_dumps()at lines667-700 refuse native crash collectors and disable core files before any grammar read. Synthetic review regressions verify raw-private-file isolation and refusal before reading.cc-crosssession-credstatus(~08:00Z): require value-free agreement between status and the runner. No host store was inspected or changed for this PR.GitTree,derive_ci_protectedandadvisory_gate_readsgenerated the candidate-specific monitoring snapshot. Source review found +5 runner/+4 setter sites, no site/shape losses, zero enforced unresolved reads, and unchanged breadth caps. No custom classifier or widened allowance was added.Evidence-class table
source_reviewcontinue/return.local_integration/syntheticlocal_integration/syntheticlocal_integrationpython3 scripts/validate.py;git diff --check.Local commands run
The two new tests cover the reported non-export defect, ASCII/control/undeclared-variable refusals, the runner's duplicate/empty/writer-grammar refusals, multiple refused lines with correct numbering, and an export-form control. Existing status expectations that prohibited all private-file reads now check the supported descriptor read, continued protection of undeclared/native files, and truthful
values_readreporting. No value is emitted in text, JSON, test results or PR evidence.Failed attempts retained: a 60-second runner capture was incomplete; an initial native runner run returned 7 failures/1 error; an intermediate combined run returned 30 failures/1 error. Unchanged reruns on the final source passed 101 and 92 tests. The cause of those transient results was not established; the passing reruns do not erase them. The metadata-only boot call resolved the separate reproducible boot-contract failures. These tests use synthetic stores and do not establish a live credential or provider operation.
Decision record
cc-crosssession-credstatus. Reuse the existing parser and reader so both tools validate through one grammar function. Inverse: revert the PR commits. No install, service change or owner store operation accompanies the fix.The individual changed contracts are declared in
docs/secret-storage.md, under "Declared test contracts for credential-status grammar (2026-10-08, PR #849)". Main's file at64a8cda01700d5e823c18e4b989e856de2ee2241, tested against the PR source, returned exactly four expected failures:CredentialStatusTests.test_undeclared_store_file_is_reported_by_name_only,test_private_file_is_ok_and_output_is_value_free,test_never_opens_or_reads_credential_files, andtest_guard_pin_check_hashes_only_the_installed_guard. The replacement checked-reader test and export-form fixtures preserve value-free output while permitting the grammar check required by the CC ruling.The unchanged baseline/category regression passed after the native snapshot refresh (1 test, 7.689 seconds). Final affected push-gate, resolver, credential-status and credential-run modules pass at merge head
772df26ea7a793e2ffc36afbc4a65b6b47739718(399 tests, 2 skips, 193.757 seconds). Before the merge commit,python3 scripts/validate.pypassed on the resolved tree: 70 components, 10,856 hashed files, 4 profiles, 232 receipts. Earlier pre-merge evidence atff18b4ad141c1c27b5b7e5cd296f2871c490d881remains separate: 398 tests, 2 skips, 213.895 seconds, and the 69-component validator result. That earlier full-module capture reported a 50-second MCP timeout while the original native process continued; its eventual rc0 and first log are retained. The final merge capture ran once and passed. Main's reviewed 321-read control was green and the pre-fold PR 330-read control was red; no standalone main baseline change was made.Host evidence
The final review repair passed 141 credential-status/credential-run tests (79.766 seconds), the focused raw-private/core-collector pair (2 tests), and the unchanged advisory baseline test (1 test, 7.142 seconds) at final composed head
6ea1adfba05b1067d12826c460307c1dae52621f. The final composition merges current main0d5672b08beb87a06daee792afeb66481d0c71e6; credential source and test inputs are byte-identical to the 141-test passing review commit. Validator rc0: 70 components, 10,870 hashed files, 4 profiles, 234 receipts. Main's credential-status test blob6c666faa014422064f8927c8ee145be0e3256e1ais unchanged between the previously tested618dd6c0 and current0d5672b0; its 44-method disjoint run against the review-fixed source returned exactly the four declared failures. Previous399-test composition evidence remains explicitly historical; these final checks validate the three review corrections.The initial raw-private regression fixture used an unsupported path placeholder and returned one missing-versus-ok failure; its corrected canonical STORE_ROOT fixture passed. The original failure is retained without credential content. No operator credential store or host core configuration was changed.
At final fixture head
31e797a9369653a8bc351d4b73e8560844c48b62, status, writer and runner modules passed together: 175 tests, rc0, 84.045 seconds under the Python3.12 CI environment. Validator rc0: 70 components, 10,870 hashed files, 4 profiles, 234 receipts; evidence registry updated last. Main's two modified test modules at0d5672b08beb87a06daee792afeb66481d0c71e6, run against the unchanged candidate production source, returned exactly the four individually declared status-method failures across 78 tests. The writer method retains identical expected values and explicitly observes metadata only.git diff --checkpassed.Hosted run37815646323 at earlier6ea1adfb is retained as a failed attempt: 20 status subtest failures plus one writer metadata failure. Its Ubuntu core collector made the new production guard correctly return core_pattern_pipe before reading. The existing temporary-store fixtures now also supply a synthetic kernel core-pattern input for in-process and subprocess main(argv) calls, following the existing runner fixture launcher at tests/test_credential_run.py:67,280-309. Default production CLI protection is unchanged, and the pipe-collector-before-read regression remains active. The writer's temporary-file test selects the existing metadata-only flag. These individual test contract/scope adjustments are named in docs/secret-storage.md; no assertion is removed or weakened. This is fixture integration evidence, not acceptance of a live credential host.
No files under
evidence/hosts/changed.Checklist