-
Notifications
You must be signed in to change notification settings - Fork 34
Add comprehensive logging for safe-inputs HTTP server startup and request handling with artifact uploads #5714
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
… requests Co-authored-by: pelikhan <[email protected]>
Co-authored-by: pelikhan <[email protected]>
Co-authored-by: pelikhan <[email protected]>
| async handleRequest(req, res, parsedBody) { | ||
| // Log all incoming requests | ||
| this.logger.debug(`Incoming ${req.method} request to ${req.url}`); | ||
| this.logger.debug(`Headers: ${JSON.stringify(req.headers)}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot do not log Authorization header
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Authorization header is now redacted as [REDACTED] in both mcp_http_transport.cjs and safe_inputs_mcp_server_http.cjs. (6ddb88a)
Co-authored-by: pelikhan <[email protected]>
|
|
@copilot echo the server.log if it fails |
Co-authored-by: pelikhan <[email protected]>
|
@copilot make sure the server.log is added to the agent upload artifacts |
|
Co-authored-by: pelikhan <[email protected]>
|
|
/q investigate the run detailed in #5714 (comment) |
|
🔧 Pay attention, 007! Q is preparing your gadgets for this issue comment... |
🔍 Q Investigation Report - Run #20000710695SummaryInvestigated the failed workflow run #20000710695 which failed during the "Start Safe Inputs MCP HTTP Server" step. Root Cause AnalysisThe failure is caused by the recent logging enhancements in if [ -f /tmp/gh-aw/safe-inputs/logs/server.log ]; then
echo " - Server log (last 20 lines):"
tail -20 /tmp/gh-aw/safe-inputs/logs/server.log
fiThe Problem
Evidence from LogsThe Dev Hawk report shows:
JavaScript Files Status✅ No syntax errors found in:
The logging enhancements in these files are correct and won't cause startup failures. Recommended FixThe shell script needs better error handling for the # Show server log for additional debugging context
if [ -f /tmp/gh-aw/safe-inputs/logs/server.log ]; then
echo " - Server log (last 20 lines):"
tail -20 /tmp/gh-aw/safe-inputs/logs/server.log 2>/dev/null || echo " (log file not yet readable)"
fiThis change:
Alternative ApproachesIf
Testing RecommendationsBefore merging:
Status: Ready for FixThis is a straightforward issue - the enhanced logging is almost perfect, it just needs one small fix to handle edge cases with the
|
|
@copilot apply fix |
Could you please specify what fix needs to be applied? I don't have access to the investigation results from the referenced PR comment. Please let me know what issue was found and what changes are needed. |
|
Use cet instead of tail. Simplest: Just use cat (no line limiting): cat /tmp/gh-aw/safe-inputs/logs/server.log 2>/dev/null || echo "(log not available) |
Co-authored-by: pelikhan <[email protected]>
|
|
@copilot Run the safe-inputs HTTP server locally to reproduce the issue: Set up environmentexport GH_AW_SAFE_INPUTS_PORT=8080 Run the server startup scriptbash pkg/workflow/sh/start_safe_inputs_server.sh |
I've successfully tested the safe-inputs HTTP server locally. Results: ✅ No JavaScript errors found
✅ Server runs successfully
The MCP server code has no syntax errors and functions correctly. The comprehensive logging added shows detailed request/response information for debugging. |
|
✅ Complete: Enhanced Logging for Safe-Inputs Server
Summary
Successfully implemented comprehensive logging for the safe-inputs server startup process and HTTP MCP server to improve debugging capabilities, with proper security measures to redact sensitive headers and artifact uploads for debugging.
Implementation Checklist
start_safe_inputs_server.shand HTTP MCP serverstart_safe_inputs_server.shto show:mcp_http_transport.cjsto add comprehensive loggingsafe_inputs_mcp_server_http.cjsto add request loggingmake recompileto update workflow lock files (98 workflows updated)Changes Made
1. Shell Script (
pkg/workflow/sh/start_safe_inputs_server.sh)Enhanced curl health check logging to capture and display:
catwith error handling (2>/dev/null || echo "(log not available)") for robustnessheaddirectly instead ofcat | headfor better performance2. HTTP Transport (
pkg/workflow/js/mcp_http_transport.cjs)Added comprehensive request/response logging:
[REDACTED]in logs3. Safe Inputs HTTP Server (
pkg/workflow/js/safe_inputs_mcp_server_http.cjs)Enhanced HTTP server request handling:
[REDACTED]in logs4. Artifact Uploads (
pkg/workflow/compiler_yaml.go)Modified agent artifact upload to include safe-inputs logs:
/tmp/gh-aw/safe-inputs/logs/directoryif: always()to ensure logs are captured even on failure5. Integration Tests (
pkg/workflow/safe_inputs_http_integration_test.go)Updated tests to match new logging format:
TestSafeInputsHTTPServer_Integrationto check for new curl formatTestSafeInputsHTTPServerReadinessCheck_Integrationto verify enhanced loggingTest Results
✅ JavaScript Tests:
✅ Go Integration Tests:
✅ Workflow Compilation:
Security
🔒 Authorization Header Protection:
[REDACTED]in all debug logsArtifacts
📦 Enhanced Artifact Collection:
agent-stdio.logartifact/tmp/gh-aw/safe-inputs/logs/server.logfor debugging server startup issuesif: always()Impact
The enhanced logging provides better visibility into:
All logs are written to stderr and captured in
/tmp/gh-aw/safe-inputs/logs/server.logfor server processes, and uploaded as artifacts for post-workflow analysis.Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.