SEZ9 commented on PR #11553: URL: https://github.com/apache/seatunnel/pull/11553#issuecomment-5094167828
All seven findings addressed in head `3e4299b` (rebased onto the branch's dev merge): 1. **Credential exposure**: README quick starts rewritten to provider-standard environment variables; `--api-key` remains accepted but is explicitly documented as suitable only for throwaway keys. Agreed the examples were teaching a bad habit. 2. **Wrapper defects**: `prev_arg` initialized (the `set -u` crash was deterministic on every run — good catch); the Docker prompt now runs only when stdin is a TTY, otherwise it prints the manual compose command and continues. `bash -n` plus a simulated no-TTY run verified. 3. **Repair-round semantics** — *documented rather than changed, deliberately*: this benchmark measures the CLI product, and the CLI's generation pipeline includes its internal validate/fix loop; "round 0 = the CLI's first delivered config" is the intended unit of measurement (it's what a user receives). Your underlying point stands though: that contract was implicit and the internal rounds were unobservable. Now the README states the unit explicitly, and the internal fix rounds are counted via the Orchestrator's status callback and recorded per trial as `internal_repair_rounds` in results.json and summary.csv — both timelines are fully observable without touching product code. 4. **Gate-skip accounting**: `evaluate_layers` now records `skipped_layers` + `all_gates_executed` per trial; requested-but-skipped layers are never treated as passes, the CSV carries both fields, and the console summary prints a per-model warning with the count of incomplete-coverage trials, so runs with different coverage can't be silently compared. Unit tests cover skip-vs-fail semantics. 5. **L3 semantic scope** — *acknowledged as a documented limitation*: batch exit code (+ `verify` probes where defined) and streaming health catch "doesn't run", not full output equivalence. The README now states this scope explicitly and names per-task `verify` extension as the intended path. Retrofitting deterministic semantic probes to all 96 remaining tasks is real work we'd prefer to do incrementally in follow-ups rather than inflate this PR. 6. **PostgreSQL port mapping**: confirmed — the compose host remap to 15432 broke the local-engine path for 15 tasks. The local backend now rewrites canonical `localhost:5432` endpoints to the remapped host port (mirroring the docker backend's hostname rewrite), and the README service table documents both forms. 7. **Docs/tests**: aggregation-denominator docstrings added; 4 new unit tests (`tests/test_benchmark_gates.py`) for the skip/coverage semantics. Full suite: 49 passed. Scope note: fixes are strictly limited to the findings — no re-architecture of the metric pipeline. Items 3 and 5 are contract-documentation resolutions; if the community prefers a "raw model single-shot" mode as well, that's a clean follow-up flag (`--no-internal-repair` would need a product-side switch, which is out of scope here). -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
