Skip to content

Add request logging middleware#9

Open
nev-offload wants to merge 1 commit intomainfrom
test/add-logging
Open

Add request logging middleware#9
nev-offload wants to merge 1 commit intomainfrom
test/add-logging

Conversation

@nev-offload
Copy link
Contributor

This PR adds a simple request logging middleware that logs incoming requests with timestamp, method, and path.

Please review the implementation.

@github-actions
Copy link

github-actions bot commented Feb 3, 2026

🤖 OpenClaw Bot

I'll help you review this PR that adds request logging middleware. Let me first examine the repository structure and the current implementation.
Let me check the repository structure and find the main source files:
Now let me examine the current implementation in the main index.ts file:
Now let me check the git diff to see exactly what was added in the PR:
Let me check the git status and log to understand the changes:
Let me check the git diff between the current state and the previous commit:
Let me try a different approach and check the git show command:
I can see the issue now. The PR added the request logger middleware but it's not actually being used in the application. Let me provide a comprehensive review:

PR Review: Add request logging middleware (#9)

Summary

This PR adds a simple request logging middleware that logs incoming requests with timestamp, method, and path. However, there's a critical issue: the middleware is defined but not actually applied to the Express app.

Issues Found

  1. Middleware not applied: The requestLogger middleware is defined but never registered with the Express app using app.use(). This means the logging functionality is not active.

  2. Type safety: The middleware uses any types for req, res, and next parameters. It would be better to use proper TypeScript types from Express.

  3. Missing error handling: The middleware doesn't handle potential errors that might occur during logging.

Suggested Improvements

The middleware should be applied to the app. Here's how it should be integrated:

import express, { Request, Response, NextFunction } from 'express';
import tasksRouter from './routes/tasks';

const app = express();
const PORT = process.env.PORT || 3000;

// Middleware
app.use(express.json());

// Request logging middleware (should be added here)
app.use((req: Request, res: Response, next: NextFunction) => {
  console.log(`[${new Date().toISOString()}] ${req.method} ${req.path}`);
  next();
});

// Routes
app.use('/tasks', tasksRouter);

// Health check
app.get('/health', (req, res) => {
  res.json({ 
    status: 'ok',
    uptime: process.uptime()
  });
});

// Root endpoint
app.get('/', (req, res) => {
  res.json({ 
    status: 'ok', 
    message: 'Demo API - Maintained by OpenClaw 🤖' 
  });
});

app.listen(PORT, () => {
  console.log(`Server running on port ${PORT}`);
});

Recommendation

The PR needs to be updated to:

  1. Actually apply the middleware to the Express app
  2. Improve TypeScript typing
  3. Consider adding request/response timing for more comprehensive logging

The middleware implementation itself is simple and appropriate for basic logging needs, but it needs to be properly integrated to be functional.

@nev-offload
Copy link
Contributor Author

OpenClaw webhook test: PR comment from subagent.

@nev-offload
Copy link
Contributor Author

OpenClaw webhook test: second PR comment to verify webhook delivery.

@github-actions
Copy link

github-actions bot commented Feb 3, 2026

🤖 OpenClaw Bot

HTTP 429: Rate limit exceeded: free-models-per-day. Add 10 credits to unlock 1000 free model requests per day

@github-actions
Copy link

github-actions bot commented Feb 3, 2026

🤖 OpenClaw Bot

HTTP 429: Rate limit exceeded: free-models-per-min.

@nev-offload
Copy link
Contributor Author

✅ Fixed: Action No Longer Hangs

Root Cause

The GitHub Action was hanging indefinitely because:

  1. No hard timeout - The entire run() function had no maximum execution time
  2. No explicit process.exit() - Node.js stayed alive if any handles (WebSocket, child process) remained open
  3. Cleanup not guaranteed - If run() hung, the gateway process was never killed
  4. Internal timeouts existed (300s) but didn't force the Node.js process to exit

The Fix

I made the following changes to offloadmywork/openclaw-github-app:

  1. Added 10-minute hard timeout - The entire action now has a maximum execution time of 10 minutes
  2. Added explicit process.exit() - The action now explicitly exits with code 0 (success) or 1 (failure)
  3. Moved cleanup to finally block - Client disconnect and gateway shutdown always happen, regardless of success/failure
  4. Increased agent timeout - Changed from 5 minutes to 8 minutes (within the 10-minute limit)
  5. Guaranteed gateway cleanup - The gateway process is always killed, even on hard timeout

Test Results

Verified working - Workflow run 21656642201 completed successfully:

  • Duration: ~3.5 minutes
  • Status: Completed with success
  • Cleanup: Gateway stopped cleanly
  • Exit: Process exited with code 0

Previous runs were stuck in_progress for 2.5+ hours. This run completed in under 4 minutes and exited cleanly.

What Changed

  • Commit: 01d7f0a
  • Files modified: src/index.ts, src/client.ts
  • The action now always exits within 10 minutes, even if:
    • The LLM API returns 429 rate limit errors
    • The agent request times out
    • The WebSocket connection doesn't close
    • The gateway process hangs

Issue resolved! 🎉

@github-actions
Copy link

github-actions bot commented Feb 4, 2026

🤖 OpenClaw Bot

HTTP 429: Rate limit exceeded: free-models-per-min.
I'll help you understand the context of this comment and check the current state of the repository.

@github-actions
Copy link

Thanks for adding the request logging middleware! This will be valuable for API observability. I'll review this PR once it's ready for consideration.

@github-actions
Copy link

Review: Add Request Logging Middleware

✅ What's Good

  • Simple Implementation: Clean, straightforward middleware
  • Useful Information: Logs method, path, and timestamp
  • Non-intrusive: Doesn't interfere with request processing
  • Good for Debugging: Will be helpful for troubleshooting

⚠️ Suggestions for Improvement

  1. Type Safety: Consider adding proper TypeScript types instead of any
  2. Middleware Registration: The middleware needs to be registered with Express
  3. Log Level: Consider using a proper logging library for production
  4. Configuration: Make logging level configurable

🔧 Implementation Issues

The middleware is defined but not actually registered with the Express app. It needs to be added to the middleware chain.

📋 Summary

Approve with Changes: The concept is good and useful, but needs to be properly integrated and typed.

Would you like me to implement these improvements and merge the PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant