test(tail): add scenarios for edge cases flagged in PR #26 review#97
Conversation
Add three missing scenario tests for security/correctness edge cases identified in the chatgpt-codex-connector review of PR #26: - malformed_offset_plus_minus: rejects -n +-3 as invalid (not valid GNU tail) - malformed_offset_plus_plus: rejects -c ++5 as invalid (not valid GNU tail) - minint64_overflow: rejects -9223372036854775808 without panicking (negation overflows back to negative in two's complement) All three cases are already handled correctly by parseCount() in tail.go; these scenarios document and protect that behaviour. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 69d0979cd2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| stdout: "" | ||
| stderr_contains: ["tail:"] | ||
| exit_code: 1 | ||
| skip_assert_against_bash: true |
There was a problem hiding this comment.
tail -n -9223372036854775808 file.txt is accepted by GNU tail (coreutils) and returns exit code 0 (it prints the file when it has fewer than that many lines), but this scenario asserts exit_code: 1 with stderr_contains and then disables bash parity via skip_assert_against_bash: true. That locks in a known divergence instead of catching it, which conflicts with the test suite’s bash-compatibility goal for non-restricted behavior.
Useful? React with 👍 / 👎.
Three fixes: 1. tail: clamp MinInt64 instead of rejecting it -(-9223372036854775808) overflows back to itself, but GNU tail accepts this value and prints the file (treating it as a large count). Change parseCount to clamp to MaxCount on overflow rather than return invalid, matching bash/GNU tail behaviour. 2. interp: remove runtime/debug.Stack from panic recovery The stack trace leaks internal details; the panic value alone is sufficient. This drops the runtime/debug import from api.go and runner_exec.go, fixing the allowedsymbols test failure. 3. interp: use io.Discard as panic fallback instead of os.Stderr When the runner has no stderr configured, the previous code fell back to os.Stderr, bypassing the embedder's I/O contract. Use io.Discard so the message is silently dropped; the caller still receives "internal error" via the returned error. This also drops os.Stderr from the interp allowlist. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep it up! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
julesmcrt
requested review from
AlexandreYang,
astuyve,
matt-dz and
thieman
as code owners
gh-worker-dd-devflow-36fce6
bot
added
mergequeue-status: queued
mergequeue-status: in_progress
and removed
mergequeue-status: queued
labels
gh-worker-dd-mergequeue-cf854d
bot
merged commit 680f119
into
main
gh-worker-dd-mergequeue-cf854d
bot
deleted the
jules.macret/re-review-tail
branch
gh-worker-dd-devflow-36fce6
bot
added
mergequeue-status: done
and removed
mergequeue-status: in_progress
labels
2 participants
Add three missing scenario tests for security/correctness edge cases identified in the chatgpt-codex-connector review of PR #26:
All three cases are already handled correctly by parseCount() in tail.go; these scenarios document and protect that behaviour.
What does this PR do?
Motivation
Testing
Checklist