docs(eval): restructure README with navigation and a troubleshooting section - #4281
docs(eval): restructure README with navigation and a troubleshooting section#4281amaldevcm wants to merge 2 commits into
Conversation
…section Adds a table of contents and section headers over the existing prose, plus two new sections with no prior coverage: the ExperimentExecutor/ SubjectAdapter extension pattern for custom executors and subjects, and a symptom-to-fix troubleshooting table sourced from the actual preflight/egress error strings in the codebase. No existing technical content was altered, only reorganized. Fixes apache#3895 Generated-by: Claude
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for restructuring this README—the navigation and troubleshooting flow make the Eval surface much easier to inspect. I found one small protocol-precision point below. It is a suggestion from an outside review, so please feel free to push back if another transport limit is intended.
AI-assisted review disclosure: Codex assisted with documentation-to-source verification; Astro-Han is the contributor of record for this review.
|
|
||
| ## Relay protocol | ||
|
|
||
| Harbor/Pier subprocess stdout is merged Docker stdout/stderr that can interleave, so a structured result cannot simply be parsed from the tail of the output. `writeRelayResult` ([`relay-result-frame.ts:24,34-43`](src/relay-result-frame.ts)) emits one bounded (under 2 KB, below Linux `PIPE_BUF`) line — `MAKA-EVAL-RESULT-V1 <token> <bytes> <sha256> <base64url payload>` — so the executor can extract a verified, size-capped result frame out of otherwise-unstructured merged output. `takeRelayResultToken` consumes a one-time environment-variable token (`MAKA_EVAL_RESULT_TOKEN`) so the frame can be authenticated per-attempt. |
There was a problem hiding this comment.
[P3] (category ① — normal documentation path)
Thanks for documenting this invariant. RESULT_PAYLOAD_LIMIT_BYTES caps the JSON payload at 2 KiB, but the complete emitted line also includes the token, length, 64-byte SHA-256 digest, prefix, and base64url expansion, so the full frame can be roughly 2.8 KiB. It still stays below Linux PIPE_BUF; could this say “the payload is capped at 2 KiB and the complete line stays below Linux PIPE_BUF” instead? Please push back if I am overlooking an additional transport-level cap.
There was a problem hiding this comment.
You're right, and no additional transport-level cap beyond PIPE_BUF. The base64url expansion alone puts the full line around 2.8 KiB. Fixed to say the payload is capped at 2 KiB and the complete line stays below Linux PIPE_BUF, without implying the whole line is under 2 KB. Pushed in 383cda9.
RESULT_PAYLOAD_LIMIT_BYTES caps only the JSON payload at 2 KiB before base64url encoding; the complete emitted line (prefix, token, length, 64-char SHA-256 digest, base64url expansion) runs closer to 2.8 KiB. Both figures stay below Linux PIPE_BUF, but "under 2 KB" was wrong for the whole line. Say what's actually capped at 2 KiB and what's below PIPE_BUF instead of conflating the two. Addresses review comment on apache#4281. Generated-by: Claude
Summary
packages/eval/README.mdalready existed and was technically accurate, but had zero section headers — one H1 title followed by 27 unbroken paragraphs, with no way to jump to "how do I run an eval" or "what do I do when it fails."This restructures the file in place:
ExperimentSpec(experiment.ts) andparseExperimentSpec(spec.ts)ExperimentExecutor/SubjectAdapterinterfaces and theEvalCliDependenciesoverride seam (runner.ts,cli.ts) had no prior documentation at allinstall-preflight.tsand the egress-audit failure modes, not paraphrasedmaka evalis a subcommand of the mainmakaCLI, not a separate binary, matching howARCHITECTURE.mdalready describes itFixes #3895
Verification
git diffshows only moves, not content changes)runner.ts,cli.ts,spec.ts,experiment.ts,install-preflight.tsmaka evalCLI-wiring claim againstpackages/cli/src/cli-core.ts, and for consistency against the rootREADME.mdandARCHITECTURE.md, both before and after rebasing onto latestmainnpx biome formatdoes not process markdown in this repo (confirmed — no formatter to run against this file)maka eval runend-to-end (would need a real spec + provider credentials); verified the command's wiring in source insteadAI use
Select exactly one:
Tool(s) and scope: Claude (Sonnet 5) — investigated the issue, explored the eval package source (dispatched as two parallel research passes covering architecture/CLI and security/egress), designed the restructuring plan, and wrote the changes.
Generated-by: Claudetrailer is on the commit.Checklist
Does this PR entail a change in behavior?