diff --git a/CLAUDE.md b/CLAUDE.md index 18c288a..60a2017 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -70,3 +70,15 @@ Dispatch enforces a **registration gate**: only CAP/PASS/NICK/USER/QUIT/PONG run ## Testing Tests use Google Test but also feed every result into **PostMan** (`vendor/PostMan.cpp`), a styled Unicode-table reporter — `tests/test_main.cpp` bridges the two via a custom `TestEventListener`. `tests/Makefile` builds all of `src/` *except* `main.cpp` (linking `tier_full.cpp` as the one `registerExtensions` definition). Protocol-level suites share `tests/TestHarness.hpp` (TCP `TestClient` + `IrcServerTest` fixture; subclass and override `portBase()` per suite, `onServerReady()` to inject probe extensions). Test files: `test_message`, `test_client`, `test_channel`, `test_bot`, `test_integration`, `test_robustness`, `test_security`, `test_filetransfer`, `test_extensions`, `test_libcpp98`. Suite is 138/138; PostMan's leak counter is atomic and `assertNoLeaks` takes `const char*` (a `std::string` argument would count itself as a leak — keep it that way). + +## Known traps + +- **RST/error teardown**: real socket errors (`ECONNRESET`, etc.) are torn down + via the `EPOLLERR|EPOLLHUP` branch in `run()`, **not** by inspecting `errno` + after `recv`/`send` — that was removed (the subject forbids errno-driven + control flow after non-blocking I/O syscalls; it also cured a latent EINTR + false-positive disconnect). Caveat for future event-loop work: which branch + reaps an RST is **timing-dependent** when `EPOLLIN` and `EPOLLHUP` arrive in + the same event — do NOT assume `EPOLLERR|HUP` alone covered RST before that + errno removal (`RobustnessTest.AbruptDisconnectViaRST` guards it now). + \ No newline at end of file diff --git a/src/Server.cpp b/src/Server.cpp index aaebf20..da93d88 100644 --- a/src/Server.cpp +++ b/src/Server.cpp @@ -230,11 +230,7 @@ void Server::acceptClient() reinterpret_cast(&clientAddr), &addrLen); if (clientFd < 0) - { - if (errno != EAGAIN && errno != EWOULDBLOCK) - Log::error(std::string("accept() failed: ") + strerror(errno)); return; - } // Connection cap: reject gracefully instead of exhausting fds if (_clients.size() >= MAX_CLIENTS) @@ -286,7 +282,7 @@ void Server::handleClientInput(int fd) ssize_t bytesRead = recv(fd, buf, MAX_MSGLEN, 0); if (bytesRead <= 0) { - if (bytesRead == 0 || (errno != EAGAIN && errno != EWOULDBLOCK)) + if (bytesRead == 0) disconnectClient(fd, "Connection closed"); return; } @@ -322,11 +318,7 @@ void Server::handleClientOutput(int fd) const std::string &buf = client->getSendBuffer(); ssize_t bytesSent = send(fd, buf.c_str(), buf.size(), 0); if (bytesSent < 0) - { - if (errno != EAGAIN && errno != EWOULDBLOCK) - disconnectClient(fd, "Send error"); return; - } client->clearSendBuffer(bytesSent); // SendQ sweep: the peer is too slow / flooded; its stream already diff --git a/tests/COVERAGE.md b/tests/COVERAGE.md new file mode 100644 index 0000000..132774d --- /dev/null +++ b/tests/COVERAGE.md @@ -0,0 +1,107 @@ +# ft_irc — Defense Coverage Map (42 correction sheet) + +Cross-references every item on the 42 evaluation sheet against what the repo +actually proves today. Built from a scan of `tests/`, `scripts/audit.sh`, and +`src/` on `main` (`4e78d69`). + +**Legend:** ✅ covered · 🟡 partial · 🔴 gap / risk · 🧭 manual-only (not automatable) + +Defend on the **`make mandatory`** binary — it excludes PlatformBus / AuditLog / +FancyLogSink, shrinking the surface an evaluator can question. + +--- + +## A. Basic checks (ELIMINATORY — any failure = grade 0) + +| Sheet item | Status | Proven by | Notes / gap | +|---|---|---|---| +| Makefile; compiles `-Wall -Wextra -Werror -std=c++98`; exec `ircserv` | ✅ | `audit.sh` §compile | Also `make mandatory/bonus/full` all build | +| **Only one** poll/epoll/select | ✅ | `audit.sh` §single-poll; verified 1 `epoll_wait` (Server.cpp:158) | PlatformBus multiplexes into the same epoll — no 2nd wait | +| poll called before each accept/recv/send | 🟡 | architectural (all I/O is epoll-event-driven) | **Exception:** best-effort `send()` in `disconnectClient` (Server.cpp:503) is not preceded by a write event → decide defend vs remove | +| **errno not used to trigger action after recv/send/accept** | 🔴 | — (unaudited, untested) | **TOP RISK.** `errno==EAGAIN` checks after recv (289), send (326), accept (234). No retry-loop, but errno branches control flow. Refactor recommended (EPOLLERR/HUP already handles real errors) | +| fcntl only `F_SETFL, O_NONBLOCK` | ✅ | `audit.sh` §fcntl; verified (Server.cpp:104,250) | PlatformBus fcntl also compliant, but it's an extra | + +--- + +## B. Networking + +| Sheet item | Status | Proven by | Notes / gap | +|---|---|---|---| +| Starts, listens on all interfaces on CLI port | 🟡 | integration binds+connects | "All interfaces" (INADDR_ANY) is a code fact; not asserted, easy to defend | +| `nc` connects, sends, gets answers | ✅ | `IntegrationTest.*` (raw TCP `TestClient` = nc-equivalent) | | +| Reference client (HexChat) connects | 🧭 | — | **Manual before defense.** Not automatable | +| Multiple simultaneous connections, non-blocking | ✅ | `Robustness.RapidConnectDisconnect`, `CommandFlood`; `EventLoopTest` (idle no-spin) | Strong | +| Join channel; broadcast to all members | ✅ | `Integration.JoinChannel/ChannelMessage`; `Channel.BroadcastToAllMembers/ExcludesSender` | | + +--- + +## C. Networking specials (robustness) + +| Sheet item | Status | Proven by | Notes / gap | +|---|---|---|---| +| Partial commands; others keep running | 🟡 | `Integration.PartialDataReassembly`; `ClientBuffer.PartialMessagePreserved`; `LineBuffer98.FragmentedCRLF` | Reassembly proven; "other clients fine *while* a partial is pending" not explicitly interleaved | +| Kill client abruptly; server stays up | ✅ | `Robustness.AbruptDisconnect`, `RapidConnectDisconnect` | | +| Kill nc mid-command | 🟡 | `Robustness.AbruptDisconnect` | Not specifically "disconnect while a partial is buffered" | +| **^Z a reader + flood from another; no hang; drains on resume; no leaks** | 🔴 | partial: `SendQ.OverflowLatches`, `CommandFlood`, `NoLeakAfterClientChurn` | **GAP.** No integration test of a *frozen reader* being flooded (output backpressure). Directly tied to the EPOLLOUT/SENDQ design — high-value test | +| No memory leaks during operations | 🟡 | in-process leak counter (`NoLeak*`, PostMan) + `scripts/memcheck.sh` exists | Eval uses **valgrind**; in-process counting ≠ valgrind. Verify `memcheck.sh` runs the sheet's scenarios (esp. ^Z+flood) under valgrind | + +--- + +## D. Client commands (basic) + +| Sheet item | Status | Proven by | Notes / gap | +|---|---|---|---| +| Auth, NICK, USER, JOIN | ✅ | `Integration.SuccessfulRegistration/WrongPassword/NoPassword/JoinChannel` | | +| PRIVMSG with different parameters | 🟡 | `Integration.PrivateMessage/ChannelMessage` | Add error cases: no such nick (401), no text (412), multi-target | + +--- + +## E. Channel operator commands (scored 0–5, −1 per broken feature) + +| Sheet item | Status | Proven by | Notes / gap | +|---|---|---|---| +| Operator CAN: KICK / INVITE / TOPIC / MODE i,t,k,o,l | ✅ | `Integration.KickUser/InviteToChannel/TopicSetAndQuery/ChannelMode{Query,Key,Limit}`; `Channel.*Mode*`; `ModeBounds*` | Happy path well covered | +| **Regular user is DENIED operator actions** | 🔴 | — | **GAP.** Sheet explicitly checks this. No negative test asserting `ERR_CHANOPRIVSNEEDED (482)` for non-op KICK/TOPIC/MODE/INVITE | +| +i / +t enforced end-to-end (uninvited JOIN blocked; non-op TOPIC blocked) | 🟡 | mode *state* tested at unit level | Enforcement path not asserted end-to-end via TCP | + +--- + +## F. Bonus (only if mandatory is perfect) + +| Sheet item | Status | Proven by | Notes / gap | +|---|---|---|---| +| File transfer with reference client | 🟡 | `FileTransfer.*` (10 tests, incl. DCC relay) | Unit coverage strong; "with reference client" is 🧭 manual | +| A bot | ✅ | `Bot.*` (10 tests) | | + +--- + +## G. Overarching (checked continuously during defense) + +| Sheet item | Status | Notes | +|---|---|---| +| No segfault / crash for the whole defense | 🟡 | Robustness suite helps; ultimately live + valgrind | +| No leaks (heap freed before exit) | 🟡 | See C — needs a valgrind harness as a defense artifact | + +--- + +## Prioritized action list (defense order) + +**P0 — eliminatory, do first (a gap here = 0, everything else is moot):** +1. **errno-after-I/O** (A). Audit → refactor recv/send/accept to not branch on + `errno` (rely on `EPOLLERR|EPOLLHUP`), or prepare an airtight justification. + Guard: `Robustness.AbruptDisconnect` must stay green. +2. **Un-polled `send()` in `disconnectClient`** (A/B). Decide: remove (accept + losing app-buffered QUIT/ERROR text) or defend as a teardown flush. + +**P1 — scored / high-value robustness:** +3. **Non-operator denial tests** (E) — the operator score is 0–5 and the sheet + checks denial explicitly. Add `ERR_CHANOPRIVSNEEDED` negative tests. +4. **Frozen-reader + flood** integration test (C) — the marquee robustness + scenario; showcases the EPOLLOUT/SENDQ backpressure design. +5. **Valgrind harness** (C/G) — verify/extend `scripts/memcheck.sh` to run the + sheet's scenarios under valgrind; keep the output as a defense artifact. + +**P2 — completeness / polish:** +6. Re-land `test_eventloop.cpp` (idle no-spin) — merged fix, missing its guard. +7. +i/+t end-to-end enforcement tests (E); PRIVMSG error cases (D). +8. **Manual reference-client (HexChat) checklist** (B/F) — the 🧭 items. diff --git a/tests/TESTING.md b/tests/TESTING.md new file mode 100644 index 0000000..aa51e12 --- /dev/null +++ b/tests/TESTING.md @@ -0,0 +1,161 @@ +# Testing & QA Discipline + +How this project turns "I fixed it" into "a test would catch it if it ever +came back." This document is the QA companion to `WORKFLOW.md`: the workflow +defines *how a change moves through audit → plan → review*; this defines *what +evidence a change must carry* and *how tests are written, validated, and kept +honest*. + +It is a **discipline, not a platform.** It rides on what already exists — the +GoogleTest suite under `tests/` and the three-agent workflow — and deliberately +adds no new framework, runner, or "QA subsystem." If you find yourself building +infrastructure to support QA, stop: that is scope creep, and this project +already carries enough of it. + +--- + +## 1. Principle: a test is evidence, not decoration + +A bug fix is not done when the symptom disappears. It is done when a test +exists that **fails on the broken code and passes on the fixed code**. That +discrimination — red before, green after — is the only thing that proves the +test actually exercises the bug rather than passing by accident. + +The corollary is uncomfortable and load-bearing: a green test proves nothing +on its own. A test that was never seen to fail might be asserting the wrong +thing, hitting the wrong code path, or be a tautology. Every regression test +must have a recorded red state. + +--- + +## 2. The Red-Green-Review loop + +This layers directly onto the three roles in `WORKFLOW.md`. The mapping is what +makes it bias-resistant: no single agent both writes the test and decides it +passes. + +| Stage | Role (from WORKFLOW.md) | Responsibility | +|-------|-------------------------|----------------| +| **Red** | Auditor | Reproduce the bug *as a failing test* in `tests/`, and show it failing. The test is the audit's reproduction made executable. | +| **Green** | Implementer | Make the test pass — **without modifying the test** — while keeping the whole suite green. | +| **Review** | Adversarial reviewer | Independently validate the *test itself* as much as the code. | + +The hard rule that prevents gaming: **the author of the fix may not edit, +weaken, or delete the test.** In a single-agent loop, TDD quietly degrades into +"write the test my code already passes." With separated roles, the implementer +inherits a test they cannot touch and must satisfy it honestly. + +The Red test is diagnosis-as-code, so writing it does not break the auditor's +"diagnose, do not fix" posture — a failing test is not a fix. + +--- + +## 3. Not every change is Red-Green + +Forcing a failing-test-first ritual onto work that has no behavioral bug +produces awkward, dishonest tests. Match the evidence to the kind of work: + +- **Bug / regression** → Red-Green. A test that reproduces the defect, shown + failing, then passing. This is the default and the strongest case. +- **Refactor** (behavior must not change) → the existing suite stays green + before and after. If coverage of the touched area is thin, *add + characterization tests first* (capturing current behavior) so the refactor + has a safety net. No new "failing" test is expected. +- **Performance / resource fix** (no wrong output, just wrong cost — e.g. the + idle-CPU spin) → a measured **before/after number** in the PR, plus a guard + test that asserts the resource property with a **generous margin** (see §5). +- **New feature** → tests for the new behavior, including its failure and edge + cases. Red-Green applies per acceptance criterion where it fits. + +If you cannot write a reliable failing test for a bug, treat that as a signal +that the bug is not yet understood — keep auditing, or document explicitly why +a deterministic test is not achievable (e.g. a genuine heisenbug). + +--- + +## 4. How tests are written here + +Match the existing style in `tests/` rather than inventing a parallel one: + +- GoogleTest, built and run via `tests/Makefile` (`make` in `tests/`). The + server is C++98; the test tier compiles at C++17 as GoogleTest requires. +- Protocol-level tests use the shared harness in `tests/TestHarness.hpp`: + - `TestClient` — a real TCP client (connect, `sendCmd`, `recvAll`, + `registerClient`, `hasNumeric`). + - `IrcServerTest` — a fixture that runs a `Server` in a background thread. + Override `portBase()` with a unique base per suite (grep existing bases to + avoid bind clashes) and `onServerReady(Server&)` to inject test-only + extensions before `run()`. +- Register every new test file in `TEST_SRCS` in `tests/Makefile`. +- Put suite-local helpers (fake extensions, etc.) in an anonymous `namespace`. +- **Assert observable behavior, not structure.** Drive the server through its + real interface (bytes on a socket, replies, numerics, loop activity via the + extension seam) and assert on what an actual client or operator would see. + "The object exists" / "the count is 1" proves existence, not behavior. + +Prefer reusing the architecture's own seams over adding production hooks for +testing. (The idle-spin regression test counts event-loop iterations through +the existing `IServerExtension::onTick` seam — it adds zero production code.) + +--- + +## 5. Keeping the suite trustworthy + +Flaky and slow tests are how a suite dies: people start ignoring red. Defend +against it: + +- **Prefer deterministic signals over timing or %CPU.** Count iterations, + inspect state, assert on returned bytes — not wall-clock thresholds. +- **When a threshold is unavoidable, demand a huge margin and document it.** + (The idle-spin guard separates ~1–2 iterations/sec from >100000 with a + threshold of 1000 — six orders of magnitude of headroom.) +- **Slow, stress, and load tests live in a separate target** (e.g. a `stress` + target), never in the default suite. Add them only when a concrete risk + justifies them — not speculatively. +- The default suite must stay fast and green. A red default suite means stop + and fix, not "known flaky, ignore." + +--- + +## 6. The reviewer's test-validity checklist + +In addition to reviewing the code (see the `adversarial-reviewer` skill), the +reviewer must vet the **test** and record the result: + +- [ ] **Discrimination, reproduced independently.** The reviewer ran the test + against the unfixed code and saw it FAIL, and against the fixed code and saw + it PASS — not trusting the author's numbers. +- [ ] **Right reason.** The red failure is caused by the actual bug, not by an + unrelated error (compile failure, wrong port, timeout). +- [ ] **Behavioral, not structural.** The assertion reflects what a user or + operator observes. +- [ ] **No tautology / no false positive.** The test can actually fail; it is + not asserting something always true. +- [ ] **Margin and runtime.** Any threshold has ample headroom; the test does + not meaningfully slow the default suite, or it lives in a separate target. +- [ ] **Untouched by the fix.** The implementer did not modify the test to make + it pass. + +A change with code that looks correct but a test that fails this checklist is +**APPROVE WITH CHANGES** at best — an unverified test is not a regression guard. + +--- + +## 7. QA as an ongoing loop + +Beyond single fixes, the same discipline scales into a repeatable habit for the +project: **audit a component → reproduce a weakness as a failing test → fix → +have the reviewer validate both → fold the lesson into `CLAUDE.md`.** Each pass +leaves the suite stronger and the institutional memory (the "known traps" in +`CLAUDE.md`) richer. + +This loop is intentionally low-tech. It is the existing suite plus the existing +three-agent workflow plus the rules above — nothing to install, nothing to +maintain. Resist the temptation to grow it into a QA product. Its value is the +discipline, not the tooling. + +--- + +*Referenced from `WORKFLOW.md`. The method is portable; adapt it. If the rules +get in the way more than they help on a given change, you are applying them to +the wrong kind of work — see the taxonomy in §3.* diff --git a/tests/test_robustness.cpp b/tests/test_robustness.cpp index c53cadf..ab0e678 100644 --- a/tests/test_robustness.cpp +++ b/tests/test_robustness.cpp @@ -144,6 +144,61 @@ TEST_F(RobustnessTest, AbruptDisconnect) close(fd2); } +TEST_F(RobustnessTest, AbruptDisconnectViaRST) +{ + /* Connect, register, then force a real RST (not a FIN) via SO_LINGER */ + int fd = quickConnect(serverPort); + ASSERT_GE(fd, 0); + + const std::string nick = "abruptrst"; + sendLine(fd, "PASS robpass"); + sendLine(fd, "NICK " + nick); + sendLine(fd, "USER abruptrst 0 * :Test"); + std::this_thread::sleep_for(std::chrono::milliseconds(200)); + recvBuf(fd); + + struct linger sl; + sl.l_onoff = 1; + sl.l_linger = 0; + setsockopt(fd, SOL_SOCKET, SO_LINGER, &sl, sizeof(sl)); + close(fd); + + /* + * Black-box proof the reset client was actually torn down server-side + * (not just that the listener still accepts): register a NEW client + * reusing the SAME nick. Nick release depends on the server processing + * EPOLLERR|HUP for the RST, which is not synchronous with our close() + * — bounded retry instead of a fixed sleep: up to 15 attempts, 100ms + * apart (1.5s total margin). Each attempt reconnects on a fresh socket + * and sends a full PASS/NICK/USER handshake, so the test makes no + * assumption about partial-registration or NICK-only retry semantics + * — success is simply seeing RPL_WELCOME (001) for that attempt. + */ + const int MAX_ATTEMPTS = 15; + bool registered = false; + std::string lastReply; + + for (int attempt = 0; attempt < MAX_ATTEMPTS && !registered; ++attempt) + { + int fd2 = quickConnect(serverPort); + ASSERT_GE(fd2, 0); + + sendLine(fd2, "PASS robpass"); + sendLine(fd2, "NICK " + nick); + sendLine(fd2, "USER abruptrst2 0 * :Test"); + std::this_thread::sleep_for(std::chrono::milliseconds(100)); + lastReply = recvBuf(fd2); + + registered = lastReply.find(" 001 ") != std::string::npos; + close(fd2); + } + + EXPECT_TRUE(registered) + << "Nick '" << nick << "' should be released after RST within " + << MAX_ATTEMPTS << " retries (100ms apart); last reply: '" + << lastReply << "'"; +} + /* ════════════════════════════════════════════════════════════════════════ * Suite: Robustness — Rapid connect / disconnect * ════════════════════════════════════════════════════════════════════ */