Repository navigation
Conversation
Add one thin, read-only pass-through to evidence.verify() so the plugin's callOp() keeps all five verdicts in result.status instead of losing four normal non-zero states to runJson()/CLI_FAILED. BRIDGE_SCHEMA_VERSION stays 1; integration_info() derives ops from _OPS. No change to verify(), EXIT_CODES, the CLI or the fingerprint. Add tests/test_evidence_verify_op.py (8 states/contract checks). Closes Core HarryHeYu#22; Core half of dsh-sessionflow#9 (Option A).
|
Confirmed — you already have this covered, so I am not asking you to claim anything: PR #24 ( Status I see: OPEN, draft, I will review it on the merits (thin wrapper only, five states reachable by name, Two notes so we do not duplicate effort:
No fixed deadline, and no need to push further commits for me — I will comment on the PR itself with the review. |
HarryHeYu
left a comment
There was a problem hiding this comment.
Review — looks correct; verified by running it
I fetched refs/pull/24/head (856a09e) into an isolated worktree and ran the new test file:
python -m pytest tests/test_evidence_verify_op.py -q
=> 8 passed in 6.13s
What I checked against the agreed boundary:
- Thin wrapper only.
evidence_verify()opens a read-onlyStoreand returnsevidence.verify(...)verbatim — no second implementation of the five states, no reinterpretation ofmessage, no local digest logic.verify()andEXIT_CODESare untouched. - Verdict is a field, not an exit code.
result.statuscarries the five names andresult.exit_coderides along as data, which is exactly why acallOp()client keepsMISMATCH/NOT_FOUND/UNVERIFIABLE/INVALID_REFERENCEinstead ofCLI_FAILED. - Schema stays 1.
BRIDGE_SCHEMA_VERSIONis unchanged and the op is registered in_OPS, sointegration_info()advertises it — feature detection by op name, no version sniffing. - Old-Core degradation is a capability error.
unknown op: ...is asserted as such, and the test explicitly keeps it distinct from an evidenceNOT_FOUND— that is the distinction I care about most here. - No scope creep. Two files, no existing line modified, no MCP tool, no CLI change, no
REQUIRED_CORE_REFbump.
One note, not a blocker: evidence_verify(db=None, session_id="", seq=None, digest=None) will happily take session_id="", and verify() will answer INVALID_REFERENCE for it. That is the honest outcome for a malformed reference, so I am fine leaving argument validation to the plugin layer (which should refuse before spawning). Just flagging that the op itself does not pre-validate.
No change requested. Keeping it draft is right; the merge decision and the "installable Core" step are mine to sequence, and I will not merge it silently. Plugin sessionflow_evidence_verify can proceed as a draft PR against this shape.
HarryHeYu
left a comment
There was a problem hiding this comment.
Final technical acceptance — PASS (no blocking issues)
Re-checked at 856a09e (unchanged since my earlier review). Still Draft,
CI 10/10 pass, MERGEABLE / CLEAN. I am not changing its status —
it stays Draft until you mark it Ready for Review.
I re-verified the seven points by running the op, not by reading alone. Seeded a
temporary index, called handle_request({"op": "evidence_verify", ...}, db=...)
for each verdict:
BRIDGE_SCHEMA_VERSION: 1
evidence_verify in _OPS: True
integration_info().ops has it: True
MATCH status=MATCH exit_code=0
MISMATCH status=MISMATCH exit_code=1
NOT_FOUND status=NOT_FOUND exit_code=2
UNVERIFIABLE status=UNVERIFIABLE exit_code=3
INVALID_REFERENCE status=INVALID_REFERENCE exit_code=4
unknown op -> error: "unknown op: 'evidence_verify_v2' (ops: [...])" # no result
EXIT_CODES: {'MATCH': 0, 'MISMATCH': 1, 'NOT_FOUND': 2, 'UNVERIFIABLE': 3, 'INVALID_REFERENCE': 4}
Against the agreed boundary:
- Thin wrapper only —
evidence_verify()opens a read-onlyStoreand
returnsevidence.verify(...)verbatim.verify(),EXIT_CODESand the
fingerprint algorithm are untouched; the diff is two files (voyager/api.py,
tests/test_evidence_verify_op.py), no existing line modified. - All five states reachable by name through the API, each with its own
status(the run above). exit_codeis a result field, not the API's process status: every call
returned{id, result}with no error, including the four non-zero verdicts.
This is the property that keepscallOp()from flattening them into
CLI_FAILED.schema_versionstays 1 — verified directly.integration_info().opsincludesevidence_verify— verified, so a
client can feature-detect by op name instead of sniffing versions.- Old-Core behaviour is a capability error, not a verdict — an unknown op
returnserrorwith noresult; it cannot be mistaken for evidence
NOT_FOUNDorMATCH. - No scope change — no CLI change, no MCP tool, no fingerprint change, no
REQUIRED_CORE_REFbump, no other provider touched.
Also ran the PR's own suite: tests/test_evidence_verify_op.py → 8 passed.
Non-blocking note
evidence_verify(db=None, session_id="", seq=None, digest=None) does not
pre-validate; an empty/malformed reference resolves to INVALID_REFERENCE,
which is the honest answer. Argument validation belongs in the plugin layer
(refuse before spawning) — no change requested here.
Merge order
Core first, then the plugin tool. This PR is the Core half; the plugin
sessionflow_evidence_verify (dsh-sessionflow#9) can be prepared as a draft now
but should not be accepted until an installable Core containing this op exists.
Not merged here. Merge is a separate explicit step after you mark it Ready;
no release or tag change is made in this round.
What
Add one additive
evidence_verifyop tovoyager/api.py_OPS— a thin,read-only pass-through to the existing
evidence.verify()— plus a dedicatedbridge-contract test (
tests/test_evidence_verify_op.py). Two files changed,no existing line modified.
Closes/advances sessionFlow#22
and is the Core half of
dsh-sessionflow#9
(Option A, confirmed by the maintainer on 2026-11-12).
Why
voyager evidence verify --jsonalready returns the five verdictsMATCH/MISMATCH/NOT_FOUND/UNVERIFIABLE/INVALID_REFERENCE,but it is CLI-only (
docs/MAINTENANCE.mdknown-issue #6). The plugin'srunJson()throwsCLI_FAILEDon any non-zero process exit code, whilevoyager/evidence.py:39-45uses exit codes 1–4 for four normal verdicts,so a direct CLI call (Option B) would discard them.
callOp()reads a singleJSON line from
voyager apiand returnspayload.result, so the verdict livesin
result.status, decoupled from the process exit code. A machine op is theonly way to keep all five states over the bridge without a second transport.
Scope — strictly additive (no expansion)
api.py: +1 import (from .evidence import verify), +1 function, +1_OPSentry.
BRIDGE_SCHEMA_VERSIONstays 1;integration_info()derivesops/capabilitiesfrom_OPS, so the new op is discoverable by name.verify()/EXIT_CODES/ the five-state semantics /FINGERPRINT_ALGORITHM(
sha256-8hex-json-v1) / CLI behaviour /resolveVoyager()/REQUIRED_CORE_REF/ MCP / peer range / CI matrices: untouched.docs/INDEX.mdregistration needed.MATCHremains "this reference is consistent", never "the claim is true".unknown op: 'evidence_verify' (ops: …)— a capability error mapped by the plugin to
OP_ERROR, never confused withevidence
NOT_FOUNDand never degraded toMATCH.Verification (level:
UNIT_TESTED)pytest tests/test_evidence_verify_op.py -q→ 8 passed. Asserts all fivestates round-trip by name through
handle_request(incl.INVALID_REFERENCEon malformedseq/ non-8-hex digest, andUNVERIFIABLEwith no digest), the{id, result}envelope, the verbatimdocumented result shape,
MATCH≠ "claim is true", schema stays 1,integration_info().opsadvertises the op, old-Coreunknown opdegradation, and the real stdio
voyager apisurface (one JSON line).Fixtures follow the real
voyager evidence verify --jsonshape (same seededindex the CLI tests use); no invented response fields.
→
bad=0, byte-identical restore. The new test has teeth.pytest tests/ -q --tb=short -rf:3 failed, 1382 passed, 8 skipped(1393 collected). The 3 failures arepre-existing / environment artifacts, not introduced here:
test_cli.py::test_version_reports_the_installed_distributionand::test_dunder_version_agrees_with_the_distribution—importlib.metadata.PackageNotFoundError: No package metadata was found for voyager(the PR is built from a source tarball, not an installed dist; reproduced
identically with the pristine
api.py).test_verification_harness_v2.py::test_a_successful_run_records_evidence—order-dependent/flaky under the full run (GBK decode warnings from a
subprocess reader thread); passes 3/3 in isolation with this change.
These are the same release-engineering/CI-environment issues the maintainer
noted in the #9 division-of-labor comment
(release workflow permission + no fixed deadline); they do not block a
feature-branch Draft PR.
scripts/check_consistency.py --strict --online: 3 checks fail onv1.3.1 not a visible git tag(the not-yet-pushed release tag), all inREADME/version files not touched by this PR; the
bridge schema agreementcheck is SKIP (no plugin root here). Same class ofpre-existing release-environment issue.
INTEGRATION_TESTED/LIVE_VERIFIEDclaim: those need an installableCore version over the real bridge, which depends on the maintainer's
release-workflow fix.
Merge order (cross-repo,
CONTRIBUTING.md)sessionflow_evidence_verifycallscallOp('evidence_verify', …);formal acceptance gated on a real-core integration test. Plugin Draft PR /
stub contract tests may be prepared in parallel, marked blocked.
This PR is opened as a Draft and is blocked on step 2 (installable Core
version) for formal acceptance, exactly as Core #22 / the maintainer's comment
allow. Keep dsh-sessionflow#9
as the design record; no fixed delivery time.
Author: SunnyRad (same as #7 / prior PRs).