Agentic bootstrap, MCP-only planning, model updates, UX improvements#27
Agentic bootstrap, MCP-only planning, model updates, UX improvements#27josephgoksu wants to merge 5 commits intomainfrom
Conversation
Security (3 medium-severity): - Block find -exec/-execdir/-delete/-ok in ExecTool (injection risk) - Add "--" before grep pattern to prevent flag injection - Replace ad-hoc path validation in MCP handlers with safepath.SafeJoin - Use safepath.SafeJoin in policy builtins resolvePath (traversal risk) - Change crash log permissions from 0644 to 0600 Code quality: - Add rows.Err() checks after all 8 rows.Next() loops in codeintel/repository.go - Replace err == io.EOF with errors.Is(err, io.EOF) in 3 files - Fix out.Close() error handling in memory_export.go (named return) Freshness review fixes (Greptile PR #26 feedback): - Fix decay discontinuity: unified formula (1.0 - ratio * 0.8) replaces hardcoded 0.2 all-missing + 0.4 formula partial-missing - Add cache eviction (purge entries >2x TTL when cache exceeds 1000) - Convert global cache to struct-based statCache (testable, bounded) - Add TODO(freshness-level2) comments for dead code paths - Mark freshness plan doc as "implemented with deviations" - Add TestDecaySmoothCurve test verifying no discontinuities
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if filepath.IsAbs(path) { | ||
| return path | ||
| // Verify absolute path is within WorkDir | ||
| absWorkDir, err := filepath.Abs(bc.WorkDir) | ||
| if err != nil { | ||
| return "", fmt.Errorf("resolve work dir: %w", err) | ||
| } | ||
| if !strings.HasPrefix(filepath.Clean(path), absWorkDir) { | ||
| return "", fmt.Errorf("path outside work directory: %s", path) | ||
| } | ||
| return path, nil | ||
| } |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| c.mu.Lock() | ||
| c.entries[path] = cacheEntry{info: info, err: err, checkedAt: time.Now()} | ||
| // Evict expired entries when cache grows too large | ||
| if len(c.entries) > cacheMaxSize { | ||
| c.evictExpired() | ||
| } | ||
| c.mu.Unlock() |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
- Updated migration tests to clarify directory structure and improve comments for better understanding. - Enhanced error handling in the TUI by treating agent completion warnings as non-fatal, allowing for smoother user experience. - Improved list view rendering by adding workspace checks and refactoring header rendering for better clarity. - Introduced new skills for TaskWing, including detailed SKILL.md files for various commands to enhance project knowledge and workflow. - Added functionality to simplify code while preserving behavior, ensuring that optimization does not bypass necessary gates. - Implemented status command to provide current task progress and acceptance criteria, improving user awareness of task status.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| } | ||
| } | ||
| // Also check for go.mod in subdirectories (monorepo) | ||
| if entries, err := os.ReadDir(basePath); err == nil { |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 days ago
General fix: ensure that any path derived from user input is validated before being used in filesystem operations, not only when joining subpaths. In this case, hasDependencyFiles should not blindly trust its basePath argument; it should verify that basePath is a valid, existing directory (and, ideally, normalized) before calling os.ReadDir or os.Stat on paths derived from it.
Best concrete fix here without changing behavior: add a minimal, defensive validation step at the start of hasDependencyFiles that rejects non-existent or non-directory basePath values. This keeps its behavior the same for legitimate calls (where projectPath is an existing project directory) but ensures that even if ValidateAbsPath were misused or bypassed elsewhere in the future, hasDependencyFiles itself will not operate on arbitrary or unexpected paths. The new check should be placed inside hasDependencyFiles in internal/bootstrap/factory.go, before any filesystem operations: use os.Stat(basePath) to confirm that it exists and IsDir() to confirm it’s a directory; if not, return false immediately. This change is self-contained, requires no new imports (we already import os), and preserves all existing functionality for valid directories.
Concretely:
-
Edit
internal/bootstrap/factory.go. -
In
hasDependencyFiles, immediately after the function signature, add:info, err := os.Stat(basePath) if err != nil || !info.IsDir() { return false }
-
Leave the rest of the logic as is, still using
safepath.SafeJoinfor combiningbasePathwith file names and still iterating dependency manifests and subdirectories.
No other files (internal/server/handlers.go, internal/bootstrap/runner.go) need changes, because handleBootstrap already validates ProjectPath with safepath.ValidateAbsPath, and the new guard makes hasDependencyFiles robust even if it’s called from other contexts.
| @@ -59,6 +59,12 @@ | ||
| // hasDependencyFiles checks if any common dependency manifest exists at the project root. | ||
| // Uses safepath.SafeJoin to prevent path traversal in basePath. | ||
| func hasDependencyFiles(basePath string) bool { | ||
| // Ensure basePath points to an existing directory before inspecting it. | ||
| info, err := os.Stat(basePath) | ||
| if err != nil || !info.IsDir() { | ||
| return false | ||
| } | ||
|
|
||
| for _, name := range dependencyManifests { | ||
| p, err := safepath.SafeJoin(basePath, name) | ||
| if err != nil { |
- Remove cmd/plan.go, cmd/goal.go, and plan TUI (1,982 lines deleted) - MCP plan tool (clarify/generate/decompose/expand/finalize/audit) unchanged - Planning prompt now scales task count dynamically to goal complexity - Tasks include self-contained context (files, patterns, constraints, tech stack) - Task enricher always provides baseline project context (constraints + decisions) - Fix stop hook reliability: sync TasksCompleted from DB on each invocation - Add session save retry logic to prevent stale state from failed writes - Clear stale CurrentTaskID when DB lookup fails
josephgoksu
changed the title
- policy/builtins.go: use safepath.ValidateAbsPath for absolute paths, fixes prefix collision bypass (e.g. /project-evil matching /project) - freshness/freshness.go: add evictOldest fallback when all cache entries are fresh but cache exceeds max size during burst scenarios - mcp/handlers.go: restore absolute path support in validateAndResolvePath via safepath.ValidateAbsPath, fixing regression for MCP clients - agents/impl/analysis_deps.go: use filepath.Join instead of string concat - bootstrap/factory.go: use safepath.SafeJoin for dependency file checks
| // hasAnyDependencyFile checks if at least one common dependency manifest exists. | ||
| func hasAnyDependencyFile(basePath string) bool { | ||
| for _, name := range commonDepFiles { | ||
| if _, err := os.Stat(filepath.Join(basePath, name)); err == nil { |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
Copilot Autofix
AI 4 days ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
| if err != nil { | ||
| continue | ||
| } | ||
| if _, err := os.Stat(p); err == nil { |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
Copilot Autofix
AI 4 days ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
…improvements - Unified GetProjectContext replaces 3 separate retrieval paths (ContextRetriever, TaskEnricher, buildTaskContext) - Bootstrap: fix duplicate "Bootstrap complete!" in multi-repo, batch link warnings into single summary, clean verification output - Bootstrap: per-service project context in workspace mode, progress callbacks, repo count accuracy - Models: add gpt-5.4/5.4-mini/5.4-nano, claude-opus-4-6/sonnet-4-6, gemini-3.1, remove deprecated models - Models: newest-first ordering in selection UI - UI: grouped knowledge output, freshness indicators, consistent header boxes across all commands - UI: markdown rendering in ask --answer output, adaptive width - Skills: add /taskwing:context, remove /taskwing:debug and /taskwing:simplify, fix stale file pruning - Skills: clarify questions presented to user (not auto-answered), auto-start first task after plan creation - Hooks: stop hook syncs TasksCompleted from DB, session save retry, verbose logs gated behind --debug - MCP: behavioral CLAUDE.md instructions tell AI tools when to use TaskWing - Prompts: generic and directive (no hardcoded tech stack examples), dynamic task count scaling - Fix: read_file tool returns helpful message for directories instead of crashing ReAct agent - Fix: auto-regenerate slash commands on MCP server start after brew upgrade - Fix: truncateString consolidated to utils.Truncate, nil guards in context.go, brief.go pluralization - Remove: CLI plan/goal/slash commands, docs/_partials, sync-docs scripts, stale doc references
| if err != nil { | ||
| continue | ||
| } | ||
| if _, err := os.Stat(p); err == nil { |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
Copilot Autofix
AI 4 days ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
1 participant
Summary
Greptile Summary
This is a large, multi-faceted PR that delivers five meaningful improvements: a two-wave agentic bootstrap with ReAct-first execution and deterministic fallback; removal of ~1,982 lines of CLI planning commands in favour of MCP-only planning; a stop-hook reliability fix syncing task completion from the DB; a model registry update adding the latest GPT-5.4/Claude-4-6/Gemini-3.1 model families; and a knowledge output redesign with grouped-by-type display.
The architectural direction is sound — the two-wave execution model is well-reasoned, the adaptive agent selection is pragmatic, and the
safepath.SafeJoinconsolidation inhandlers.gowas overdue. Most changes are clean and well-commented.Issues found:
P1 — debug log prints empty string after clearing task ID (
cmd/hook.go):session.CurrentTaskIDis zeroed out before being referenced in the verbose debug log, so the log always prints"[DEBUG] Could not load current task : <err>"— the original ID is silently lost, making this log useless for diagnosing stale references.P1 — fallback loop for unknown knowledge types is dead code (
internal/mcp/presenter.go): The loop intended to render nodes whose types are absent fromtypeOrderchecksgrouped[node.Type]instead of thetypeOrderslice. Becausegroupedis built from allresult.Results, every node type is present in the map, so the condition!okis always false. Any future knowledge type not intypeOrder(e.g.,"risk") will be silently dropped from MCPaskresponses.P2 — duplicated dependency manifest list (
internal/agents/impl/analysis_deps.go/internal/bootstrap/factory.go):commonDepFilesanddependencyManifestsare identical slices defined in two separate packages. These will diverge as new ecosystems are added.P2 — non-portable path join (
internal/agents/impl/analysis_deps.go):hasAnyDependencyFileusesbasePath + "/" + nameinstead offilepath.Join, inconsistent with all other file-path handling in the codebase.P2 — wave 1 errors silently dropped (
internal/bootstrap/runner.go): The_discard ofrunParallel's error return for wave 1 provides zero diagnostic visibility when thedocordepsagents fail; at minimum a log line should surface this in verbose mode.Confidence Score: 3/5
Important Files Changed
Sequence Diagram
sequenceDiagram participant CLI as tw bootstrap participant Factory as bootstrap.Factory participant Wave1 as Wave 1 (doc + deps) participant Wave2 as Wave 2 (code + git) participant ReAct as ReAct Agent (Eino) participant Det as Deterministic Chain CLI->>Factory: NewDefaultAgents(cfg, path, snap) Factory-->>CLI: adaptive agent list CLI->>Wave1: runParallel(ctx, wave1Agents, input) Wave1->>ReAct: runReactMode(systemPrompt, userMsg, maxSteps) alt ReAct succeeds (≥ threshold findings) ReAct-->>Wave1: findings else ReAct fails / below threshold Wave1->>Det: DeterministicChain.Invoke() Det-->>Wave1: findings end Wave1-->>CLI: wave1Results (errors silently discarded) CLI->>CLI: buildWaveContext(wave1Results) CLI->>Wave2: runParallel(ctx, wave2Agents, input + wave1Context) Wave2->>ReAct: runReactMode(systemPrompt, userMsg, maxSteps) alt ReAct succeeds ReAct-->>Wave2: findings else fallback Wave2->>Det: DeterministicChain.Invoke() Det-->>Wave2: findings end Wave2-->>CLI: wave2Results CLI-->>CLI: append(wave1Results, wave2Results) CLI->>CLI: printPostBootstrapSummary()Prompt To Fix All With AI
Last reviewed commit: "refactor(plan): remo..."