Skip to content

Flag runner-refused credential store lines - #849

Merged
seathatflowsinourveins merged 8 commits into
mainfrom
codex/ns2604-credential-status-grammar-20261008
Oct 8, 2026
Merged

seathatflowsinourveins merged 8 commits into
mainfrom
codex/ns2604-credential-status-grammar-20261008

Conversation

@seathatflowsinourveins

@seathatflowsinourveins seathatflowsinourveins commented Oct 8, 2026 •

Copy link
Copy Markdown
Collaborator

Scope

The credential status command reported a private store as ok even when the key runner refused its first line. It now reports every grammar refusal as line N: reason_code and marks the entry unsafe, while valid export-form stores remain ok.

  • Current-main composition: 618dd6c05ff6750705b52426261748d11e003477, merged into this branch at 772df26ea7a793e2ffc36afbc4a65b6b47739718 before final affected-module verification.
  • Original runner source base: 7a3637f5df7111fda87f2f1408e23963d18caadc.
  • Lane: lane:foundation.
  • Owned paths: credential status and runner, its existing status test module, boot recorder call, secret-storage documentation, the OpenHands monitoring baseline, and registered evidence hashes.
  • Separate from 2604 drift fixes #846; credential paths still require the CC's explicit landing cue.

The runner's existing per-line body is extracted into one parse_line function used by both tools. The runner still stops at its first refusal; status collects all refused lines. Status uses the runner's exact injectable() predicate, invokes its core-dump guard before the checked file-descriptor reader, and reports completed grammar reads with values_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

  • Existing credential runner at the base: its supported checked reader, LINE pattern, parse at lines252-285, and writer-grammar checks. The extracted line body has the same AST as that body except the former comment/blank-line continue becomes helper return; no grammar is reimplemented in status.
  • Existing credential writer: the runner continues using its BARE_VALUE and encode checks.
  • CPython 3.13 os reference: the existing reader uses dir_fd, O_NOFOLLOW, fstat, bounded read, and descriptor cleanup. This PR reuses that reader.
  • Existing runner safety helpers: injectable() at line151 selects only runner-eligible private env files; check_core_pattern() and disable_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 decision cc-crosssession-credstatus (~08:00Z): require value-free agreement between status and the runner. No host store was inspected or changed for this PR.
  • Existing OpenHands resolver at the reviewed source head: native GitTree, derive_ci_protected and advisory_gate_reads generated 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

Claim Evidence class Command / receipt
Same grammar implementation and refusal ordering source_review Base runner body versus extracted shared function; exact AST parity except continue/return.
Non-export/other refused lines become unsafe without content in output local_integration / synthetic Two new tests in the existing status module, using temporary fake stores only.
Existing writer, runner and boot behavior local_integration / synthetic Existing affected modules below; boot and runner test files unchanged.
Registered hashes and repository consistency local_integration python3 scripts/validate.py; git diff --check.

Local commands run

$ python3 -m unittest tests.test_credential_status.CredentialStatusTests.test_runner_refused_store_lines_are_unsafe_and_value_free tests.test_credential_status.CredentialStatusTests.test_export_form_store_stays_ok_and_value_free
Before the fix: exit 1; invalid-file test failed its eight cases and CLI verdict; export-form control passed. No synthetic content appeared in the returned output.
$ python3 -m unittest tests.test_runtime_worker_openhands_push_gate tests.test_runtime_worker_openhands_resolver tests.test_credential_status tests.test_credential_run
399 tests, OK (skipped=2); exit 0; 193.757 seconds, under the repository's Python 3.12 CI environment at the composed merge head
$ python3 scripts/validate.py
{"components": 70, "hashed_files": 10856, "profiles": 4, "receipts": 232, "status": "passed"}
exit 0; integrity and scope checks only
$ git diff --check
exit 0

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_read reporting. 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 at 64a8cda01700d5e823c18e4b989e856de2ee2241, 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, and test_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.py passed on the resolved tree: 70 components, 10,856 hashed files, 4 profiles, 232 receipts. Earlier pre-merge evidence at ff18b4ad141c1c27b5b7e5cd296f2871c490d881 remains 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 main 0d5672b08beb87a06daee792afeb66481d0c71e6; 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 blob 6c666faa014422064f8927c8ee145be0e3256e1a is 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 at 0d5672b08beb87a06daee792afeb66481d0c71e6, 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 --check passed.

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

  • No workflow changes; GitHub Actions pin/permission requirements are unaffected.
  • No credential values or new required secrets added to public evidence.
  • No paid hosting, subscription or billing surface introduced.
  • Peer-owned files/worktrees preserved; evidence manifest is in the final commit.

@seathatflowsinourveins seathatflowsinourveins added the lane:foundation Foundation lane: Claude/Codex setup, hosts, memory, RAG, research, workers label Oct 8, 2026
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 8, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-08T08:36:10.873093Z 0ca21c3 PR opened
ℹ️ 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" or "@codex security review".

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 chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread scripts/credential_status.py Resolved
Comment thread scripts/credential_status.py Resolved
Comment thread scripts/credential_status.py Resolved
@seathatflowsinourveins
seathatflowsinourveins merged commit 89e8985 into main Oct 8, 2026
35 of 36 checks passed
@seathatflowsinourveins
seathatflowsinourveins deleted the codex/ns2604-credential-status-grammar-20261008 branch October 8, 2026 18:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lane:foundation Foundation lane: Claude/Codex setup, hosts, memory, RAG, research, workers

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant