fix: close correctness gaps found by post-refactor adversarial review

Confirmed by a 17-agent review pass over the refactor diff (12 findings
verified against the code and reproduced end-to-end):

- marshalCallArgs: BYREF arguments must match the declared parameter
  type exactly (was: silent bit-reinterpretation, e.g. REAL passed to
  BYREF INTEGER printed garbage with exit 0); whole-array arguments are
  rejected (was: callee assignment smashed the array data pointer ->
  runtime SIGSEGV)
- Null guards for CHR/LCASE/UCASE/IS_NUM/NUM_TO_STR/STR_TO_NUM/ASC
  argument emission (was: compiler SIGSEGV on e.g. CHR(undeclared)),
  matching the guards MID/RIGHT/LEFT already had
- ParseReturnStmt: propagate expression-parse failure (was: diagnostic
  printed but exit 0) and recognize bare RETURN before
  ENDWHILE/UNTIL/NEXT (was: bogus diagnostic on legal early return)
- emitPrototype rejects FUNCTION/PROCEDURE names that collide with
  runtime/libc symbols (was: FUNCTION strlen silently hijacked the
  string runtime; arity mismatch crashed createArgumentAllocas) or with
  builtins (was: silently shadowed); conflicting redefinitions keep the
  first signature
- FOR STEP is evaluated exactly once, before the loop (was: once per
  iteration; the runtime-direction refactor had moved it to N+1)
- RETURN values are coerced to the declared return type via the
  signature table (RETURN n in a RETURNS REAL function now compiles)
- '&' coerces its operands to STRING (CHAR/number operands work);
  OUTPUT of a PROCEDURE call reports a clean error

AGENTS docs updated to match. All positive samples remain byte-identical
to the pre-refactor baseline.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

8 files changed, 135 insertions, 45 deletions

diff --git a/AGENTS/CONVENTIONS.md b/AGENTS/CONVENTIONS.md
index 6d6bb81..333af36 100644
--- a/AGENTS/CONVENTIONS.md
+++ b/AGENTS/CONVENTIONS.md
@@ -26,19 +26,19 @@ identifier as a type name except `VOID` (rejected outside a RETURNS clause via i
 `AllowVoid` flag); other unknown types only fail (softly) in CodeGen.
 
 **Fixed IR representation per type.** INTEGER = `i64`, REAL = `double`, BOOLEAN = `i1`,
-CHAR = `i8`, STRING = opaque `ptr` to a NUL-terminated buffer. Much code infers
-pseudocode types *backwards* from LLVM types (`getExprTypeInfo`, `ArithmeticHandler`'s
-`isIntegerTy(64)` tests, BYREF detection), so these widths are ABI.
+CHAR = `i8`, STRING = opaque `ptr` to a NUL-terminated buffer. Some code still infers
+pseudocode types *backwards* from LLVM types (`getExprTypeInfo`'s fallback,
+`ArithmeticHandler`'s `isIntegerTy(64)` tests), so these widths are ABI.
 
 **Strings are malloc'd and never freed — by design.** Every string-producing operation
 (`&`, MID/LEFT/RIGHT/LCASE/UCASE, NUM_TO_STR, CHR, CHAR/number→STRING coercion) mallocs
 a fresh buffer; string literals and BOOLEAN→STRING coercion instead return pointers to
 shared module globals, so a STRING value may alias a global. `free` is never declared or
-called. Don't "fix" leaks piecemeal; it's a global design decision. All heap-string
-operations (including `&` concatenation, `StringHandler::emitConcat`) live in
-`StringHandler`; `CodeGen` also keeps its own `MallocFunc` for the char→STRING coercion
-and INPUT STRING buffers — declared in `SetupExternalFunctions`, never fetched with a
-bare `getFunction("malloc")`.
+called. Don't "fix" leaks piecemeal; it's a global design decision. Heap-string
+operations live in `StringHandler` (including `&` concatenation, `emitConcat`) and
+`StringConversionHandler` (NUM_TO_STR/STR_TO_NUM buffers); `CodeGen` also keeps its own
+`MallocFunc` for the char→STRING coercion and INPUT STRING buffers — declared in
+`SetupExternalFunctions`, never fetched with a bare `getFunction("malloc")`.
 
 **One flat symbol namespace, one table.** `Symbols` (name → `SymbolInfo{Storage,
 TypeName, IsArray}`) in CodeGen is the only symbol table; register through
@@ -63,12 +63,14 @@ continues; the failure is always diagnosed and reflected in the exit code.
 **Calls dispatch on the declared signature.** `FunctionGen::emitPrototype` records every
 function's pseudocode signature (`FuncSig`: return type name + per-param type name and
 BYREF flag); `CodeGen::marshalCallArgs` — the single call-marshalling path for both
-call-expr and call-stmt — requires BYREF arguments to be bare variables (their alloca is
-passed) and coerces BYVAL arguments to the declared parameter type. STRING works on both
-sides in both modes. Top-level prototypes are pre-registered before statement emission,
-so call-before-definition is fine; a call with no registered signature is a compile
-error. Function names get no mangling — a pseudocode `FUNCTION printf` collides with
-libc.
+call-expr and call-stmt — requires BYREF arguments to be bare variables of *exactly* the
+declared type (their alloca is passed), coerces BYVAL arguments to the declared
+parameter type, and rejects whole-array arguments. RETURN values are coerced to the
+declared return type. STRING works on both sides in both modes. Top-level prototypes are
+pre-registered before statement emission, so call-before-definition is fine; a call with
+no registered signature is a compile error. Function names get no mangling, so
+`emitPrototype` rejects names that collide with runtime/libc symbols
+(`isReservedRuntimeName`) or with builtins.
 
 **Builtins are plain identifiers backed by one table.** All 11 builtins
 (LENGTH/MID/RIGHT/LEFT/LCASE/UCASE/ASC/CHR/IS_NUM/NUM_TO_STR/STR_TO_NUM) parse as
diff --git a/AGENTS/EXTENDING.md b/AGENTS/EXTENDING.md
index d5f9469..f6cf1b7 100644
--- a/AGENTS/EXTENDING.md
+++ b/AGENTS/EXTENDING.md
@@ -15,15 +15,17 @@ already terminated the block.
 
 **New builtin function:** add a `{Name, Arity, ReturnTypeName}` row to the static
 `Builtins` table at the top of `CodeGen.cc` (single source of truth — arity checking in
-`emitExpr` and the return type in `getExprTypeInfo` both read it), then implement IR
-emission as a method on the fitting handler (String/Char/StringConversion) and add the
-name intercept in `emitExpr`'s CallExprAST section. Builtins are plain identifiers —
-nothing is needed in lexer/parser.
+`emitExpr`, the return type in `getExprTypeInfo`, and the reject-user-redefinition check
+all read it), then implement IR emission as a method on the fitting handler
+(String/StringConversion) or inline, and add the name intercept in `emitExpr`'s
+CallExprAST section — null-check every emitted argument before use, like the existing
+branches do. Builtins are plain identifiers — nothing is needed in lexer/parser.
 
 **New binary operator:** token (if multi-char) → one line in `BinopPrecedence`
 (Parser ctor; levels: OR=3, AND=5, comparisons=10, `+ - &`=20, `* / DIV MOD`=40;
-precedence must be > 0) → branch in `ArithmeticHandler::emitBinaryOp`, which owns *all*
-binary ops and dispatches on raw LLVM operand types.
+precedence must be > 0) → branch in `ArithmeticHandler::emitBinaryOp`, which dispatches
+on raw LLVM operand types. Exception: `&` is intercepted upstream in `CodeGen::emitExpr`
+(operands coerced to STRING, then `StringHandler::emitConcat`).
 
 **New type:** register in `TypeSystem::registerBuiltins` (correct `ElementSize` —
 arrays malloc `count * ElementSize`) + `TypeKind` case + `getZeroValue` +
diff --git a/AGENTS/PITFALLS.md b/AGENTS/PITFALLS.md
index cd8cc92..967d577 100644
--- a/AGENTS/PITFALLS.md
+++ b/AGENTS/PITFALLS.md
@@ -13,7 +13,8 @@ test programs.
   `&`. Similarly INPUT only accepts a plain identifier — no `INPUT arr[i]`.
 - **Arrays cannot cross function boundaries.** `ArrayTable` is never scoped; metadata
   SSA values belong to the declaring function, so referencing another function's array
-  produces invalid IR, and arrays can't be parameters.
+  produces invalid IR. Passing a whole array as a call argument is rejected with a
+  compile error.
 - **`/` always yields REAL** (double FDiv), even for two INTEGERs; integer division is
   DIV/MOD only. CHR returns a STRING (2-byte heap buffer), not CHAR. STR_TO_NUM always
   returns REAL. NUM_TO_STR formats i64 with sprintf `"%d"` — corrupts values over 32
@@ -27,7 +28,8 @@ test programs.
   `compile()` and makes the driver exit non-zero.
 - The FOR loop variable must be pre-DECLAREd as INTEGER; FOR reuses that alloca. The
   loop direction is decided at run time from the STEP value's sign; STEP is evaluated
-  once per iteration in the condition block.
+  exactly once, before the loop. (The TO expression is still re-evaluated every
+  iteration.)
 
 ## Not implemented (9618 features)
 
diff --git a/include/cps/CodeGen.h b/include/cps/CodeGen.h
index 4124e65..140d1f5 100644
--- a/include/cps/CodeGen.h
+++ b/include/cps/CodeGen.h
@@ -79,6 +79,7 @@ public:
     void print();
     bool hadError() const { return HadError; }
     void reportError(const char *Fmt, ...);
+    static bool isBuiltinName(const std::string &Name);
     
     llvm::Value *emitExpr(ExprAST *Expr);
     void emitStmt(StmtAST *Stmt);
diff --git a/lib/CodeGen/CodeGen.cc b/lib/CodeGen/CodeGen.cc
index 31a0274..ffe10b1 100644
--- a/lib/CodeGen/CodeGen.cc
+++ b/lib/CodeGen/CodeGen.cc
@@ -45,6 +45,10 @@ const BuiltinInfo *findBuiltin(const std::string &Name) {
 
 CodeGen::~CodeGen() = default;
 
+bool CodeGen::isBuiltinName(const std::string &Name) {
+    return findBuiltin(Name) != nullptr;
+}
+
 void CodeGen::reportError(const char *Fmt, ...) {
     va_list Args;
     va_start(Args, Fmt);
@@ -500,6 +504,9 @@ Value *CodeGen::emitExpr(ExprAST *Expr) {
         Value *R = emitExpr(Bin->getRHS());
         if (!L || !R) return nullptr;
         if (Bin->getOp() == '&') {
+            L = coerceValueToType(L, resolveType("STRING"));
+            R = coerceValueToType(R, resolveType("STRING"));
+            if (!L || !R) return nullptr;
             return StrHandler->emitConcat(L, R);
         }
         return ArithHandler->emitBinaryOp(Bin->getOp(), L, R, Bin->getLine());
@@ -536,13 +543,18 @@ Value *CodeGen::emitExpr(ExprAST *Expr) {
             return StrHandler->emitLeft(Str, Len);
         }
         if (Name == "LCASE") {
-            return StrHandler->emitLCase(emitExpr(Call->getArgs()[0].get()));
+            Value *Str = emitExpr(Call->getArgs()[0].get());
+            if (!Str) return nullptr;
+            return StrHandler->emitLCase(Str);
         }
         if (Name == "UCASE") {
-            return StrHandler->emitUCase(emitExpr(Call->getArgs()[0].get()));
+            Value *Str = emitExpr(Call->getArgs()[0].get());
+            if (!Str) return nullptr;
+            return StrHandler->emitUCase(Str);
         }
         if (Name == "ASC") {
             Value *ArgVal = emitExpr(Call->getArgs()[0].get());
+            if (!ArgVal) return nullptr;
             const TypeInfo *ArgType = getExprTypeInfo(Call->getArgs()[0].get());
             Value *CharVal = nullptr;
             if (ArgType && ArgType->isChar()) {
@@ -555,14 +567,18 @@ Value *CodeGen::emitExpr(ExprAST *Expr) {
         }
         if (Name == "CHR") {
             Value *IntVal = coerceValueToType(emitExpr(Call->getArgs()[0].get()), resolveType("INTEGER"));
+            if (!IntVal) return nullptr;
             Value *CharVal = Builder->CreateTrunc(IntVal, Type::getInt8Ty(*TheContext), "chr_val");
             return coerceValueToType(CharVal, resolveType("STRING"));
         }
         if (Name == "IS_NUM") {
-            return StrConvHandler->emitIsNum(emitExpr(Call->getArgs()[0].get()));
+            Value *Str = emitExpr(Call->getArgs()[0].get());
+            if (!Str) return nullptr;
+            return StrConvHandler->emitIsNum(Str);
         }
         if (Name == "NUM_TO_STR") {
             Value *NumV = emitExpr(Call->getArgs()[0].get());
+            if (!NumV) return nullptr;
             bool IsReal = NumV->getType()->isDoubleTy();
             if (NumV->getType()->isIntegerTy(8)) {
                 NumV = coerceValueToType(NumV, resolveType("INTEGER"));
@@ -570,7 +586,9 @@ Value *CodeGen::emitExpr(ExprAST *Expr) {
             return StrConvHandler->emitNumToStr(NumV, IsReal);
         }
         if (Name == "STR_TO_NUM") {
-            return StrConvHandler->emitStrToNum(emitExpr(Call->getArgs()[0].get()), true);
+            Value *Str = emitExpr(Call->getArgs()[0].get());
+            if (!Str) return nullptr;
+            return StrConvHandler->emitStrToNum(Str, true);
         }
 
         std::vector<Value*> Args;
@@ -602,18 +620,36 @@ bool CodeGen::marshalCallArgs(const std::string &Callee,
         bool IsByRef = Sig->Params[i].second;
         ExprAST *ArgExpr = ArgExprs[i].get();
 
+        // Parameters can never be arrays, so a whole-array argument is always
+        // a user error (and would smash the data pointer if let through).
+        if (auto *Var = dynamic_cast<VariableExprAST*>(ArgExpr)) {
+            const SymbolInfo *Info = getSymbolInfo(Var->getName());
+            if (Info && Info->IsArray) {
+                reportError("Cannot pass whole array %s to %s; pass an element like %s[i]",
+                            Var->getName().c_str(), Callee.c_str(), Var->getName().c_str());
+                return false;
+            }
+        }
+
         if (IsByRef) {
             auto *Var = dynamic_cast<VariableExprAST*>(ArgExpr);
             if (!Var) {
                 reportError("BYREF argument to %s must be a variable", Callee.c_str());
                 return false;
             }
-            Value *Ptr = getNamedValue(Var->getName());
-            if (!Ptr) {
+            const SymbolInfo *Info = getSymbolInfo(Var->getName());
+            if (!Info) {
                 reportError("Unknown variable %s in BYREF call", Var->getName().c_str());
                 return false;
             }
-            Out.push_back(Ptr);
+            // BYREF aliases the storage, so the types must match exactly.
+            if (Info->TypeName != TypeName) {
+                reportError("BYREF argument %s to %s must be of type %s (got %s)",
+                            Var->getName().c_str(), Callee.c_str(),
+                            TypeName.c_str(), Info->TypeName.c_str());
+                return false;
+            }
+            Out.push_back(Info->Storage);
         } else {
             Value *V = coerceValueToType(emitExpr(ArgExpr), resolveType(TypeName));
             if (!V) return false;
@@ -716,6 +752,15 @@ void CodeGen::emitForStmt(ForStmtAST *Stmt) {
     Value *Alloca = Symbol->Storage;
     Builder->CreateStore(StartVal, Alloca);
 
+    // STEP is evaluated exactly once, before the loop.
+    Value *StepVal = nullptr;
+    if (Stmt->getStep()) {
+        StepVal = coerceValueToType(emitExpr(Stmt->getStep()), resolveType("INTEGER"));
+        if (!StepVal) return;
+    } else {
+        StepVal = ConstantInt::get(*TheContext, APInt(64, 1));
+    }
+
     BasicBlock *CondBB = BasicBlock::Create(*TheContext, "forcond", TheFunction);
     BasicBlock *LoopBB = BasicBlock::Create(*TheContext, "forloop", TheFunction);
     BasicBlock *IncBB = BasicBlock::Create(*TheContext, "forinc", TheFunction);
@@ -728,14 +773,6 @@ void CodeGen::emitForStmt(ForStmtAST *Stmt) {
     Value *EndVal = coerceValueToType(emitExpr(Stmt->getEnd()), resolveType("INTEGER"));
     if (!EndVal) return;
 
-    Value *StepVal = nullptr;
-    if (Stmt->getStep()) {
-        StepVal = coerceValueToType(emitExpr(Stmt->getStep()), resolveType("INTEGER"));
-        if (!StepVal) return;
-    } else {
-        StepVal = ConstantInt::get(*TheContext, APInt(64, 1));
-    }
-
     // The loop direction depends on the STEP value at run time.
     Value *StepNonNeg = Builder->CreateICmpSGE(StepVal,
                                                ConstantInt::get(*TheContext, APInt(64, 0)),
@@ -795,6 +832,11 @@ void CodeGen::emitStmt(StmtAST *Stmt) {
         Value *RetVal = nullptr;
         if (Ret->getRetVal()) {
             RetVal = emitExpr(Ret->getRetVal());
+            Function *F = Builder->GetInsertBlock()->getParent();
+            if (const FuncSig *Sig = FuncGen->getSignature(F->getName().str())) {
+                RetVal = coerceValueToType(RetVal, resolveType(Sig->ReturnTypeName));
+                if (!RetVal) return;
+            }
         }
         FuncGen->emitReturn(Ret, RetVal);
         return;
@@ -885,6 +927,10 @@ void CodeGen::emitStmt(StmtAST *Stmt) {
             reportError("Cannot infer the type of the OUTPUT expression");
             return;
         }
+        if (TypeInfo->isVoid()) {
+            reportError("Cannot OUTPUT the result of a PROCEDURE call");
+            return;
+        }
         emitOutputValue(Val, TypeInfo, true);
         return;
     }
diff --git a/lib/CodeGen/FunctionGen.cc b/lib/CodeGen/FunctionGen.cc
index 2e5a704..b778bbe 100644
--- a/lib/CodeGen/FunctionGen.cc
+++ b/lib/CodeGen/FunctionGen.cc
@@ -1,10 +1,24 @@
 #include "cps/FunctionGen.h"
-#include "llvm/IR/Verifier.h"
+#include "cps/CodeGen.h"
 #include <cstdio>
 
 using namespace llvm;
 using namespace cps;
 
+// Symbols the compiler itself declares (libc + runtime); a user
+// FUNCTION/PROCEDURE with one of these names would hijack the runtime.
+static bool isReservedRuntimeName(const std::string &Name) {
+    static const char *Reserved[] = {
+        "main", "printf", "scanf", "malloc", "free", "strlen", "memcpy",
+        "strcpy", "strcat", "toupper", "tolower", "sprintf", "strtol",
+        "strtod", "memset", "exit",
+    };
+    for (const char *R : Reserved) {
+        if (Name == R) return true;
+    }
+    return false;
+}
+
 Type *FunctionGen::getLLVMType(const std::string &TypeName) {
     Type *Resolved = Types.getLLVMType(TypeName);
     if (!Resolved) {
@@ -45,6 +59,19 @@ const FuncSig *FunctionGen::getSignature(const std::string &Name) const {
 }
 
 Function *FunctionGen::emitPrototype(PrototypeAST *Proto) {
+    if (isReservedRuntimeName(Proto->getName())) {
+        fprintf(stderr, "Error: '%s' is a reserved runtime symbol and cannot be a FUNCTION/PROCEDURE name\n",
+                Proto->getName().c_str());
+        HadError = true;
+        return nullptr;
+    }
+    if (CodeGen::isBuiltinName(Proto->getName())) {
+        fprintf(stderr, "Error: '%s' is a built-in function and cannot be redefined\n",
+                Proto->getName().c_str());
+        HadError = true;
+        return nullptr;
+    }
+
     FuncSig Sig;
     Sig.ReturnTypeName = Proto->getReturnType();
 
@@ -57,15 +84,24 @@ Function *FunctionGen::emitPrototype(PrototypeAST *Proto) {
         ArgTypes.push_back(T);
         Sig.Params.emplace_back(std::get<1>(Arg), std::get<2>(Arg));
     }
-    Signatures[Proto->getName()] = std::move(Sig);
 
     Type *RetType = getLLVMType(Proto->getReturnType());
     FunctionType *FT = FunctionType::get(RetType, ArgTypes, false);
     Function *F = Module.getFunction(Proto->getName());
+    if (F && F->getFunctionType() != FT) {
+        fprintf(stderr, "Error: Function %s conflicts with an existing declaration of a different type\n",
+                Proto->getName().c_str());
+        HadError = true;
+        return nullptr;
+    }
     if (!F) {
         F = Function::Create(FT, Function::ExternalLinkage, Proto->getName(), &Module);
     }
 
+    // Keep the first registration: on a duplicate definition the first one
+    // wins and the second is rejected by emitFunctionDef.
+    Signatures.emplace(Proto->getName(), std::move(Sig));
+
     unsigned Idx = 0;
     for (auto &Arg : F->args()) {
         if (Idx < Proto->getArgs().size()) {
@@ -78,11 +114,7 @@ Function *FunctionGen::emitPrototype(PrototypeAST *Proto) {
 Function *FunctionGen::emitFunctionDef(FunctionDefAST *FuncAST,
                                        const std::function<void(StmtAST*)> &StmtEmitter) {
     PrototypeAST *Proto = FuncAST->getProto();
-    Function *TheFunction = Module.getFunction(Proto->getName());
-
-    if (!TheFunction) {
-        TheFunction = emitPrototype(Proto);
-    }
+    Function *TheFunction = emitPrototype(Proto);
 
     if (!TheFunction) return nullptr;
     if (!TheFunction->empty()) {
diff --git a/lib/CodeGen/StringHandler.cc b/lib/CodeGen/StringHandler.cc
index 783f8e5..546506d 100644
--- a/lib/CodeGen/StringHandler.cc
+++ b/lib/CodeGen/StringHandler.cc
@@ -128,6 +128,7 @@ Value *StringHandler::emitLeft(Value *Str, Value *Len) {
 // Shared loop for LCASE/UCASE: malloc a copy of Str with every byte run
 // through CaseFn (tolower/toupper).
 Value *StringHandler::emitCaseConvert(Value *Str, FunctionCallee CaseFn) {
+    if (!Str) return nullptr;
     Value *Len = emitLength(Str);
     Value *One = ConstantInt::get(Context, APInt(64, 1));
     Value *AllocSize = Builder.CreateAdd(Len, One);
diff --git a/lib/Parser/ParserFunction.cc b/lib/Parser/ParserFunction.cc
index 6ce0b8e..7663f38 100644
--- a/lib/Parser/ParserFunction.cc
+++ b/lib/Parser/ParserFunction.cc
@@ -137,8 +137,12 @@ std::unique_ptr<StmtAST> Parser::ParseCallStmt() {
 std::unique_ptr<StmtAST> Parser::ParseReturnStmt() {
     getNextToken();
     std::unique_ptr<ExprAST> Expr = nullptr;
-    if (CurTok != tok_endif && CurTok != tok_else && CurTok != tok_endfunction && CurTok != tok_endprocedure) {
+    // A bare RETURN is recognized right before any block terminator.
+    if (CurTok != tok_endif && CurTok != tok_else && CurTok != tok_endfunction &&
+        CurTok != tok_endprocedure && CurTok != tok_endwhile && CurTok != tok_until &&
+        CurTok != tok_next && CurTok != tok_eof) {
         Expr = ParseExpression();
+        if (!Expr) return nullptr;
     }
     return std::make_unique<ReturnStmtAST>(std::move(Expr));
 }