Skip to content

docs(eval): restructure README with navigation and a troubleshooting section - #4281

Open
amaldevcm wants to merge 2 commits into
apache:mainfrom
amaldevcm:docs/eval-readme-structure
Open

docs(eval): restructure README with navigation and a troubleshooting section#4281
amaldevcm wants to merge 2 commits into
apache:mainfrom
amaldevcm:docs/eval-readme-structure

Conversation

@amaldevcm

Copy link
Copy Markdown

Summary

packages/eval/README.md already 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:

  • Adds a table of contents and 8 section headers over the existing prose (Architecture, Experiment spec format, Running an evaluation, Extending, Relay protocol, Docker harness profiles, Egress audit and security model, Troubleshooting)
  • No existing technical content was altered — only reorganized. Every relocated paragraph was diffed against the original to confirm it survived verbatim.
  • Adds a spec-format field table sourced from ExperimentSpec (experiment.ts) and parseExperimentSpec (spec.ts)
  • Adds a new Extending: custom executors and subjects section — the ExperimentExecutor/SubjectAdapter interfaces and the EvalCliDependencies override seam (runner.ts, cli.ts) had no prior documentation at all
  • Adds a new Troubleshooting section as a symptom → cause → fix table, built from the actual error strings in install-preflight.ts and the egress-audit failure modes, not paraphrased
  • One-line clarification that maka eval is a subcommand of the main maka CLI, not a separate binary, matching how ARCHITECTURE.md already describes it

Fixes #3895

Verification

  • Diffed the restructured file against the original to confirm every relocated paragraph survived verbatim (git diff shows only moves, not content changes)
  • Cross-checked every new claim (interfaces, error strings, field list) against the current source: runner.ts, cli.ts, spec.ts, experiment.ts, install-preflight.ts
  • Cross-checked the maka eval CLI-wiring claim against packages/cli/src/cli-core.ts, and for consistency against the root README.md and ARCHITECTURE.md, both before and after rebasing onto latest main
  • npx biome format does not process markdown in this repo (confirmed — no formatter to run against this file)
  • Not done: did not execute maka eval run end-to-end (would need a real spec + provider credentials); verified the command's wiring in source instead

AI use

Select exactly one:

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

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: Claude trailer is on the commit.

Checklist

  • Tests cover the change and fail without it — N/A, documentation-only change
  • Lint, format, typecheck and the affected suites pass locally — no code changed; markdown isn't covered by this repo's formatter/typecheck

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

…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
@github-actions github-actions Bot added the effort/M Under 500 readable lines label Aug 30, 2026

@Astro-Han Astro-Han 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.

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.

Comment thread packages/eval/README.md Outdated

## 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.

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.

[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.

@amaldevcm amaldevcm Aug 30, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/M Under 500 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

docs: add eval framework documentation (packages/eval/README.md)

2 participants