Skip to content

fix: make test suite and tools portable across default encodings and python -OO (#484) - #540

Merged
imran-siddique merged 2 commits into
agentrust-io:mainfrom
drk-ram:drk-ram-fix-utf8-portability-and-python-oo-484
Oct 19, 2026
Merged

imran-siddique merged 2 commits into
agentrust-io:mainfrom
drk-ram:drk-ram-fix-utf8-portability-and-python-oo-484

Conversation

@drk-ram

@drk-ram drk-ram commented Oct 15, 2026

Copy link
Copy Markdown
Contributor

What this changes

Closes #484.

Contributor: Raghuvar Durga (@drk-ram)

Makes the test suite, conformance suite, fixture generators, and repository tooling portable across non-UTF-8 default encodings (LC_ALL=C PYTHONUTF8=0 / Windows cp1252) and python -OO (PYTHONOPTIMIZE=2), and raises the referencing dependency floor so it is satisfiable alongside jsonschema>=4.26.0, per @imran-siddique's scope ruling on #484:

  1. Explicit UTF-8 file and subprocess encodings:
    • Adds encoding="utf-8" to all bare Path.read_text() and Path.write_text() calls across tests/ (26 files), conformance/tests/ (8 files, including conformance/tests/test_manifest_ref_draft_vectors.py and conformance/tests/vectors/manifest-ref-v0.3-draft/generate.py with newline="\n"), and examples/verifier-token-conformance/gen_corpus.py.
    • Adds encoding="utf-8" to subprocess.run(..., text=True) in tools/review_bar.py so git diff -z path output and pytest output decode as UTF-8 under LC_ALL=C PYTHONUTF8=0.
    • Cleans four em dashes in docstrings of conformance/tests/test_measurement_harness.py so python tools/check_dashes.py continues to pass once that file is edited past its conformance/import-manifest.json snapshot.
  2. Guard __doc__ under python -OO (PYTHONOPTIMIZE=2):
    • Guards description=(__doc__ or "").split("\n", 1)[0] in tools/review_bar.py and description=(__doc__ or "").splitlines()[0] if __doc__ else "" in tools/check_dashes.py.
  3. Raise referencing floor to >=0.28.4:
    • Updates referencing>=0.28 to referencing>=0.28.4 in pyproject.toml and requirements/dev.in to match jsonschema>=4.26.0's requirement.
  4. CI portability check:
    • Adds a step in .github/workflows/ci.yml running LC_ALL=C PYTHONUTF8=0 pytest and PYTHONOPTIMIZE=2 pytest tests/test_review_bar.py.

Cold-reader review:

  • Ran an independent code-review sub-agent over the full diff against the Test suite and tools are not portable across default encodings and python -OO; referencing floor is unsatisfiable #484 scope and verified via AST inspection that zero bare .read_text() or .write_text() calls remain anywhere in the repository; the cold reader reported no issues.
  • Running tests/test_review_bar.py under PYTHONUTF8=0 during verification exposed that tools/review_bar.py's git() helper also used subprocess.run(..., text=True) without encoding="utf-8", causing test_a_test_path_git_would_quote_is_still_found to misdecode UTF-8 paths emitted by git diff -z; added encoding="utf-8" there as well.

Type of change

  • Editorial (typo, link fix, clarification: no normative effect)
  • Non-breaking spec change (new optional field, new platform profile, informative addition)
  • Breaking spec change (requires 14-day comment period and Project Lead sign-off)
  • Schema change
  • Example addition

Spec section

None (test suite, tooling, CI workflow, and package dependency metadata only).

Tests for a fix

Not a library or schema verification fix (src/agentrust_trace/ and schema/ are unchanged), but the portability fixes are directly exercised by existing tests that fail on main when the changes are switched off:

  • PYTHONUTF8=0 pytest tests/test_regex_surface_parity.py tests/test_schema_regex_semantics.py tests/test_build_provenance_depth_doc.py and PYTHONUTF8=0 pytest conformance/tests/test_manifest_ref_draft_vectors.py fail with UnicodeDecodeError / fixture mismatch without the explicit encoding="utf-8" reads and pass with them (conformance/tests: 778 passed, 5 xpassed; root tests/: 3,734 passed).
  • PYTHONOPTIMIZE=2 pytest tests/test_review_bar.py -k "test_the_command_line_writes_the_summary_and_never_fails_the_build or test_an_error_inside_the_tool_still_writes_a_report_and_exits_zero" and python -OO tools/check_dashes.py fail with AttributeError: 'NoneType' object has no attribute 'split' / 'splitlines' without the __doc__ guards and pass with them.
  • ruff check src tests scripts, python tools/check_dashes.py, and mypy src/agentrust_trace all pass.

Checklist

  • DCO sign-off on all commits (git commit -s)
  • CHANGELOG.md updated (for any normative change)
  • Breaking changes marked with <!-- CHANGED: #NNN: description --> in spec text
  • Backward compatibility statement included (for breaking changes)

@drk-ram
drk-ram requested review from a team, lywinged and rajnisht7 as code owners October 15, 2026 22:23

@rajnisht7 rajnisht7 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Encoding fix looks good, fixing encoding in several conformance/tests/ files (conftest, anchor/manifest-ref vectors, measurement harness, unit loader/runner/CLI, generator). The new CI step is:

LC_ALL=C PYTHONUTF8=0 pytest
PYTHONOPTIMIZE=2 pytest tests/test_review_bar.py

Root pyproject.toml has testpaths = ["tests"], so that only runs root tests/. conformance-ci.yml still runs under a normal UTF-8 locale. So the conformance portability fixes aren’t regression-protected.
kindly add a non-UTF-8 run for conformance as well (in ci.yml or conformance-ci.yml), e.g. something that actually executes conformance/tests under LC_ALL=C PYTHONUTF8=0.

Also minor:

  • PYTHONOPTIMIZE=2 only exercises test_review_bar.py. tools/check_dashes.py was also fixed for -OO but isn’t run under -OO in CI, worth adding PYTHONOPTIMIZE=2 python tools/check_dashes.py.
  • In review_bar.py, prefer description=(__doc__ or "").split("\n\n")[0] so the -OO guard doesn’t change the existing “first paragraph” split behavior.

imran-siddique
imran-siddique previously approved these changes Oct 16, 2026

@imran-siddique imran-siddique left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Approving 68b778c. This is the scope agreed on #484. The red test (3.11) and (3.12) on 8803282 was the new LC_ALL=C job doing its job on the non-ASCII filename test, and 68b778c skips that one case. On Windows cp1252 the git-quoted path test fails on #537, #538 and #539 heads and passes here. python -OO tools/check_dashes.py --help and tools/review_bar.py --help crash with AttributeError on 2bbecec and work here. Ruff and check_dashes are clean.

Comment thread tests/test_review_bar.py Resolved
Comment thread tools/check_dashes.py Resolved
Comment thread tools/review_bar.py Resolved

@dinakarjs dinakarjs left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Independent bounded portability review at 68b778c. No blocking finding in the review-bar/tool-help boundaries tested here.

Fresh Python 3.12.14 results for tests/test_review_bar.py:

  • Normal interpreter: 52 passed.
  • LC_ALL=C PYTHONUTF8=0: 51 passed, 1 skipped. The skip is narrowly conditional on os.fsencode refusing the non-ASCII filename; the normal run exercises that case.
  • PYTHONOPTIMIZE=2: 52 passed, with pytest's expected warning about non-test assertions being disabled.
  • Both python -OO tools/check_dashes.py --help and python -OO tools/review_bar.py --help exit successfully.

For a separate regression-sensitivity control, I compiled each tool with optimize=2 in memory, restoring only its original unguarded doc access. Both reproduced AttributeError; the guarded versions exited 0 for --help. No source edits or mutations were committed. The initial in-memory probe needed its temporary module registered for dataclass introspection; the reported results are from the corrected probe.

Ruff passes for tools/review_bar.py and tests/test_review_bar.py. Including tools/check_dashes.py additionally reports two E501 lines (56 and 70) already identical on the merge base; that broader lint scope is not claimed clean. Diff whitespace passes; checkout remains unchanged. Hosted exact-head CI, Conformance CI, Review bar, docs/package, Workflow lint, fuzz and CodeQL workflows are green.

This is not a fresh full-reference/conformance suite run or native Windows test, nor a review of every altered fixture generator. GitHub currently reports the PR unmergeable against main; please reconcile its integration conflict before merge, retaining the agreed #484 scope and running CI on the resulting head. The existing maintainer review and approval authority remain separate.

drk-ram and others added 2 commits October 18, 2026 11:22
…python -OO (agentrust-io#484)

- Add explicit encoding="utf-8" to bare Path.read_text() and Path.write_text()
  calls in tests/, conformance/tests/ (including test_manifest_ref_draft_vectors.py
  and manifest-ref-v0.3-draft/generate.py), and
  examples/verifier-token-conformance/gen_corpus.py, and decode git/pytest
  subprocess output as UTF-8 in tools/review_bar.py.
- Guard module __doc__ access under python -OO (PYTHONOPTIMIZE=2) in
  tools/review_bar.py and tools/check_dashes.py.
- Raise the referencing dependency floor from >=0.28 to >=0.28.4 in
  pyproject.toml and requirements/dev.in so it is satisfiable alongside
  jsonschema>=4.26.0.
- Add a CI portability step in .github/workflows/ci.yml running
  LC_ALL=C PYTHONUTF8=0 pytest and PYTHONOPTIMIZE=2 pytest tests/test_review_bar.py.

Closes agentrust-io#484

Signed-off-by: Raghuvar Durga <177662415+drk-ram@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…path (agentrust-io#484)

Under LC_ALL=C PYTHONUTF8=0 on POSIX, sys.getfilesystemencoding() is 'ascii'
with 'surrogateescape', so os.fsencode("test_été.py") raises UnicodeEncodeError
before git is ever reached. Guard test_a_test_path_git_would_quote_is_still_found
with os.fsencode() so it skips cleanly in the ASCII C-locale portability step
while still running in the default UTF-8 test step.

Signed-off-by: Raghuvar Durga <177662415+drk-ram@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@drk-ram
drk-ram force-pushed the drk-ram-fix-utf8-portability-and-python-oo-484 branch from 68b778c to 4f170fd Compare October 18, 2026 17:23
@drk-ram

drk-ram commented Oct 18, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto origin/main (e1560f3) and addressed the review feedback from @imran-siddique and @rajnisht7:

  • tests/test_review_bar.py: updated the pytest.skip(...) reason in test_a_test_path_git_would_quote_is_still_found to state explicitly that the LC_ALL=C portability job is the one place this test does not run while the normal UTF-8 test run exercises it.
  • tools/check_dashes.py: simplified the -OO docstring guard to description=__doc__.splitlines()[0] if __doc__ else "".
  • tools/review_bar.py: preserved the first-paragraph split with description=__doc__.split("\n\n")[0] if __doc__ else "".
  • .github/workflows/ci.yml: expanded the Portability (non-UTF-8 locale and python -OO) step to also run conformance/tests under LC_ALL=C PYTHONUTF8=0 and invoke PYTHONOPTIMIZE=2 python tools/check_dashes.py --help and PYTHONOPTIMIZE=2 python tools/review_bar.py --help.

@noah-ing noah-ing left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Reviewed current head 4f170fd. The rebase addresses the earlier portability feedback: C-locale CI includes conformance, both tools run under -OO, and the review-bar help text retains its first paragraph. In an isolated Python 3.12 environment, C-locale focused root tests had 133 passes and two macOS worktree-cleanup failures that also reproduce on unchanged main; conformance had 781 passes and 5 xpasses. The -OO review-bar run passed 50 tests with those two cleanup tests excluded. Ruff, mypy, dash check, and diff whitespace check passed. Current-head hosted technical jobs pass. I found no blocking defect.

Nonblocking description correction: tests/test_acta_receipt_verifier.py merged on the base still has default-encoding Path calls, so the claim that zero bare calls remain throughout the repository is no longer literally true. Its current data is ASCII and the portability jobs pass. This Reviewer approval does not satisfy the named-maintainer gate.

@rajnisht7 rajnisht7 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Reviewed the head 4f170fd, changes looks good overall, encoding fixes, -OO guards, dependency bump, and the CI portability job all check out. The non-ASCII filename skip under C locale is the right call.

One completeness note(can be followed up): tests/test_acta_receipt_verifier.py still has a few bare read_text() / write_text() calls. Current fixtures are ASCII so nothing breaks under LC_ALL=C, but adding encoding="utf-8" there would keep the whole suite consistent and avoid the same class of bug if a non-ASCII fixture shows up later.

@dinakarjs dinakarjs left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Re-reviewed exact rebased head 4f170fd91a1b029f200b38801e29177b74764034. The expanded C-locale conformance CI, both -OO tool invocations, first-paragraph help text and narrowly conditional non-ASCII filename skip address the earlier review comments. Current-head hosted CI test jobs pass. The named-maintainer approval workflow remains separate and failed when checked; I did not treat it as a technical test failure.

Fresh Linux/Python 3.12.14 environment with the head's hash-pinned reference and conformance dependencies: C-locale conformance has 781 passes and 5 xpasses. Review-bar tests have 52 passes normally and 52 under PYTHONOPTIMIZE=2 (with pytest's expected assertion warning); both -OO --help commands work. Ruff on src/tests/scripts, source mypy, dash and whitespace checks pass.

One additional local portability gap: the C-locale root run has 4,189 passes, 49 skips and one failure at test_the_javascript_side_agrees. Node 24.19.0's test process exits 0 and emits UTF-8 checkmarks, but subprocess text decoding uses ASCII and raises UnicodeDecodeError. The same subprocess call is unchanged on base e1560f3; this is an inherited environment-dependent test-output problem, not a new verifier regression. The output remains non-ASCII with Node-related overrides removed and with CI=true.

A DCO-signed two-file companion is ready for author adoption on this exact head:
dinakarjs@272e09b

It adds explicit UTF-8 to both Node subprocess reads and the seven remaining Acta Path reads/writes identified in the other reviews. No fixtures, verifier, dependencies or workflow change. With this companion, the full C-locale root suite passes: 4,190 passed, 49 skipped. The normal focused Acta/integer selection passes 266 tests; repository lint/type/style checks pass. Restoring only the locale-default Node-output decode makes its existing test fail again with UnicodeDecodeError; restoring the fix passes. For the Acta follow-up, an isolated copied expected.json with an unused non-ASCII note fails at import under the old default read and succeeds with explicit UTF-8; the unchanged ASCII control passes. No committed fixture was altered.

Published companion tree 39efa5a3c78b5573009270c3a456c82ad4f604a2 equals the tested tree. Please incorporate or explicitly defer the companion and refresh the PR description's zero-bare-calls claim, which is stale after the rebase. I am leaving a COMMENT rather than replacing current maintainer judgments; the earlier bounded tool review stands, and this supplies the new executed evidence and repair. No native Windows execution, full -OO-library safety or protocol conformance is claimed. Implemented/reviewed with OpenAI Codex assistance.

@imran-siddique
imran-siddique merged commit 8ee6f08 into agentrust-io:main Oct 19, 2026
28 of 34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Test suite and tools are not portable across default encodings and python -OO; referencing floor is unsatisfiable

5 participants