🧹 [code health] Remove obsolete TODO for PhaseResult Output in engine_hooks - #40
🧹 [code health] Remove obsolete TODO for PhaseResult Output in engine_hooks#40eshanized wants to merge 4 commits into
Conversation
The `workflow.PhaseResult` does not have an `Output` field; instead, it has multiple specific fields depending on the phase (e.g. `Messages`, `Tasks`, `Demonstration`, `Usage`, etc.). Since there is no single `Output` field in `workflow.PhaseResult`, the output is intentionally omitted for now. This commit removes the obsolete TODO and the `Output: nil` line from `toExtensionsPhaseResult` in `internal/engine/workflow/engine_hooks.go`. Co-authored-by: eshanized <148610067+eshanized@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
…aluating symlinks The CI checks were failing because: 1. `go test -race` requires `CGO_ENABLED=1`. The CI workflow was trying to run it with `CGO_ENABLED=0`. I've updated `.github/workflows/ci.yml` to set `CGO_ENABLED: "1"` for the `-race` tests, but left it as `"0"` for all builds. 2. In `internal/tools/fileops/pathhelpers.go`, tests were failing on MacOS runners because temporary directories contain symlinks (`/var` -> `/private/var`), causing `strings.HasPrefix` to fail even when the path was correctly contained in the working directory. Added `filepath.EvalSymlinks` to both `resolved` and `workDir` to fix this issue. Also includes the removal of the obsolete TODO for PhaseResult Output in engine_hooks that was part of the original request. Co-authored-by: eshanized <148610067+eshanized@users.noreply.github.com>
…NABLED Also remove obsolete TODO for PhaseResult Output in engine_hooks and properly resolve symlinked temp paths during tests in fileops. Co-authored-by: eshanized <148610067+eshanized@users.noreply.github.com>
1 participant
🎯 What: The code health issue addressed is the removal of an obsolete TODO comment and
Output: nilassignment ininternal/engine/workflow/engine_hooks.go.💡 Why:
workflow.PhaseResultdoes not have anOutputfield; instead, it has multiple specific fields depending on the phase (e.g.Messages,Tasks,Demonstration,Usage, etc.). Since there is no singleOutputfield inworkflow.PhaseResult, the output is intentionally omitted for now, which makes the TODO obsolete. This improves maintainability by removing obsolete comments and dead code.✅ Verification: I verified the change by running the full test suite (
mkdir -p ~/.cache/gotmp && env GOTMPDIR=~/.cache/gotmp make testandmake check) and obtained a successful code review. The change is safe as it doesn't affect functionality, it just removes a comment and a nil assignment to a struct that naturally defaults to nil.✨ Result: The obsolete TODO has been removed, improving the codebase readability and maintainability.
PR created automatically by Jules for task 17170850558113440886 started by @eshanized
Summary by cubic
Removed an obsolete TODO and the dead
Output: nilmapping ininternal/engine/workflow/engine_hooks.go. Fixed CI across OSes by settingCGO_ENABLEDper step and making path containment checks follow symlinks.go test -racewithCGO_ENABLED="1"; keep builds/debug/benchmarks at"0"via per-step env in.github/workflows/ci.yml.ContainedInWorkDir(internal/tools/fileops/pathhelpers.go) to handle paths like/var→/private/varon macOS.Written for commit a373026. Summary will update on new commits.