feat(codegen): wire entrypoint execution into CLI and MCP generators#22
feat(codegen): wire entrypoint execution into CLI and MCP generators#22
Conversation
…onstruction Replace the echo stub in the Go CLI template with real exec.CommandContext calls to the manifest's entrypoint path. Build a cliArgs slice following the runner convention: positional args first, flags in definition order, auth token last. Task 1 of codegen-entrypoint-wiring. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…h args construction Replace the TODO stub in the MCP tool template with real execFile calls to the manifest's entrypoint path. Build an args array following the runner convention: positional args first, flags in definition order, auth token last via CLI flag (constitution rule 24). Task 2 of codegen-entrypoint-wiring. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…erators Verify generated CLI and MCP projects use real entrypoints instead of echo/TODO stubs. Cover per-tool entrypoint isolation, cliArgs/args construction, token passing via CLI flags, no-auth tool isolation, and TypeScript brace balance. Task 3 of codegen-entrypoint-wiring. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Greptile SummaryThis PR wires real entrypoint execution into both the Go CLI and TypeScript MCP code generators, replacing the previous echo/TODO stubs with Key changes:
Issues found:
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant CLICmd as Generated CLI Command (RunE)
participant MCPHandler as Generated MCP Tool Handler
participant Entrypoint as Entrypoint Process
Note over CLICmd: Go CLI path
User->>CLICmd: cobra RunE invoked
CLICmd->>CLICmd: Parse positional args (argXxx)
CLICmd->>CLICmd: Resolve auth token (env var → flag)
CLICmd->>CLICmd: Guard: if entrypoint == "" → error
CLICmd->>CLICmd: Build cliArgs [positional..., --flag val..., --tokenFlag token]
CLICmd->>Entrypoint: exec.CommandContext(ctx, entrypoint, cliArgs...)
Entrypoint-->>CLICmd: stdout / binary output
CLICmd-->>User: Output (text or file)
Note over MCPHandler: TypeScript MCP path
MCPHandler->>MCPHandler: Guard: if !entrypoint → throw Error
MCPHandler->>MCPHandler: Read envToken from process.env
MCPHandler->>MCPHandler: Build args [positional..., --flag val..., --tokenFlag token]
MCPHandler->>Entrypoint: await execFile(entrypoint, args)
Entrypoint-->>MCPHandler: stdout (text) or Buffer (binary)
MCPHandler-->>User: MCP content response
Last reviewed commit: 12771ab |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| import { execFile as execFileCb } from "node:child_process"; | ||
| import { promisify } from "node:util"; | ||
| {{- if .HasAuth}} | ||
| import { validateRequest } from "../auth/middleware.js"; | ||
| {{- end}} | ||
|
|
There was a problem hiding this comment.
Unused import generates dead code in every auth-enabled tool
The generated TypeScript file imports validateRequest from ../auth/middleware.js whenever HasAuth is true, but validateRequest is never called anywhere in the generated handle_ function or register function. The handler instead resolves the token directly from process.env.
validateRequest is designed for HTTP-level middleware (it inspects IncomingMessage.headers), which is not applicable inside an MCP tool handler callback. Since strict: true in the generated tsconfig.json does NOT automatically enable noUnusedLocals, this won't fail TypeScript compilation — but it will fail any ESLint/no-unused-vars setup, and it's misleading dead code in every generated tool file that has auth.
The import should either be removed from the tool template, or (if request-level auth is intended) validateRequest should actually be wired into the server transport layer in index.ts, not imported into each individual tool file.
| import { execFile as execFileCb } from "node:child_process"; | |
| import { promisify } from "node:util"; | |
| {{- if .HasAuth}} | |
| import { validateRequest } from "../auth/middleware.js"; | |
| {{- end}} | |
| {{- if .HasAuth}} | |
| {{- end}} |
Or, if the intent is to keep it for future use, add a comment explaining why it's imported but not used in the handler.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/codegen/mcp_typescript.go
Line: 684-689
Comment:
**Unused import generates dead code in every auth-enabled tool**
The generated TypeScript file imports `validateRequest` from `../auth/middleware.js` whenever `HasAuth` is true, but `validateRequest` is never called anywhere in the generated `handle_` function or `register` function. The handler instead resolves the token directly from `process.env`.
`validateRequest` is designed for HTTP-level middleware (it inspects `IncomingMessage.headers`), which is not applicable inside an MCP tool handler callback. Since `strict: true` in the generated `tsconfig.json` does NOT automatically enable `noUnusedLocals`, this won't fail TypeScript compilation — but it will fail any ESLint/no-unused-vars setup, and it's misleading dead code in every generated tool file that has auth.
The import should either be removed from the tool template, or (if request-level auth is intended) `validateRequest` should actually be wired into the server transport layer in `index.ts`, not imported into each individual tool file.
```suggestion
{{- if .HasAuth}}
{{- end}}
```
Or, if the intent is to keep it for future use, add a comment explaining why it's imported but not used in the handler.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Re: Unused validateRequest import — Fixed in 7c2a046.
validateRequest is HTTP middleware (req: IncomingMessage, res: ServerResponse, next) for Express-style request handling. MCP tool handler callbacks receive parsed JSON input, not HTTP objects — the function is not applicable here. Removed the import from the tool template. The function remains exported from auth/middleware.ts for users who wire custom HTTP transports.
internal/codegen/cli_go.go
Outdated
| if "{{$entrypoint | esc}}" == "" { | ||
| return fmt.Errorf("{{$toolName}}: entrypoint not configured") | ||
| } |
There was a problem hiding this comment.
Entrypoint guard is a compile-time constant, generating dead code for non-empty entrypoints
The guard if "{{$entrypoint | esc}}" == "" bakes the entrypoint string directly into the condition. For any tool whose entrypoint is non-empty (the common case), the template renders to:
if "/path/to/entrypoint" == "" { // always false — dead code
return fmt.Errorf("tool-name: entrypoint not configured")
}This is an unreachable branch that staticcheck will flag as SA9003 ("empty branch") and that adds noise to every generated tool file. The guard correctly protects the empty-entrypoint case (it evaluates to if "" == "" which is true) but leaves dead code for every properly-configured tool.
A cleaner approach is to hoist the check to the generator level (return an error from buildToolData or Generate when entrypoint is empty) rather than emitting a constant-false guard in the generated source. Alternatively, conditionally emit the guard only when the entrypoint IS empty — but that would mean the generated binary always fails, which is the intent, so a codegen-time error is cleaner.
The same pattern appears in the MCP template at if (!"{{.Entrypoint | esc}}") which similarly compiles to if (!"/path/to/cmd") — always false.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/codegen/cli_go.go
Line: 781-783
Comment:
**Entrypoint guard is a compile-time constant, generating dead code for non-empty entrypoints**
The guard `if "{{$entrypoint | esc}}" == ""` bakes the entrypoint string directly into the condition. For any tool whose entrypoint is non-empty (the common case), the template renders to:
```go
if "/path/to/entrypoint" == "" { // always false — dead code
return fmt.Errorf("tool-name: entrypoint not configured")
}
```
This is an unreachable branch that `staticcheck` will flag as SA9003 ("empty branch") and that adds noise to every generated tool file. The guard correctly protects the empty-entrypoint case (it evaluates to `if "" == ""` which is `true`) but leaves dead code for every properly-configured tool.
A cleaner approach is to hoist the check to the generator level (return an error from `buildToolData` or `Generate` when entrypoint is empty) rather than emitting a constant-false guard in the generated source. Alternatively, conditionally emit the guard only when the entrypoint IS empty — but that would mean the generated binary always fails, which is the intent, so a codegen-time error is cleaner.
The same pattern appears in the MCP template at `if (!"{{.Entrypoint | esc}}")` which similarly compiles to `if (!"/path/to/cmd")` — always `false`.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Fixed in 7c2a046. Both templates now assign the entrypoint to a local variable (entrypoint := "..." in Go, const entrypoint = "..." in TS), then guard and call via the variable. No more constant-expression branches in generated output.
| {{- if .HasAuth}} | ||
| // Auth token last (via CLI flag, constitution rule 24) | ||
| args.push("{{.TokenFlag | esc}}", envToken); | ||
| {{- end}} |
There was a problem hiding this comment.
TokenFlag is used as-is in MCP but normalized in CLI, creating inconsistent behavior
In buildTSToolData, TokenFlag is assigned directly from the manifest:
TokenFlag: auth.TokenFlag,In buildToolData (CLI generator), it is normalized:
tokenFlag := strings.TrimPrefix(auth.TokenFlag, "--")and the template re-adds --:
cliArgs = append(cliArgs, "--{{$tokenFlag | esc}}", token)
In the MCP template, TokenFlag is emitted verbatim:
args.push("{{.TokenFlag | esc}}", envToken);This means:
- Manifest with
TokenFlag: "--api-key"→ MCP emitsargs.push("--api-key", envToken)✓ - Manifest with
TokenFlag: "api-key"(no--) → MCP emitsargs.push("api-key", envToken)✗ (child process won't recognize it as a flag), while the CLI generator would correctly emit"--api-key".
The MCP generator should either normalize TokenFlag the same way the CLI generator does (strip -- in buildTSToolData and re-add it in the template), or document that the manifest must always include the -- prefix. The current asymmetry will silently produce a broken child process invocation for manifests that omit the prefix.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/codegen/mcp_typescript.go
Line: 757-760
Comment:
**`TokenFlag` is used as-is in MCP but normalized in CLI, creating inconsistent behavior**
In `buildTSToolData`, `TokenFlag` is assigned directly from the manifest:
```go
TokenFlag: auth.TokenFlag,
```
In `buildToolData` (CLI generator), it is normalized:
```go
tokenFlag := strings.TrimPrefix(auth.TokenFlag, "--")
```
and the template re-adds `--`:
```
cliArgs = append(cliArgs, "--{{$tokenFlag | esc}}", token)
```
In the MCP template, `TokenFlag` is emitted verbatim:
```ts
args.push("{{.TokenFlag | esc}}", envToken);
```
This means:
- Manifest with `TokenFlag: "--api-key"` → MCP emits `args.push("--api-key", envToken)` ✓
- Manifest with `TokenFlag: "api-key"` (no `--`) → MCP emits `args.push("api-key", envToken)` ✗ (child process won't recognize it as a flag), while the CLI generator would correctly emit `"--api-key"`.
The MCP generator should either normalize `TokenFlag` the same way the CLI generator does (strip `--` in `buildTSToolData` and re-add it in the template), or document that the manifest must always include the `--` prefix. The current asymmetry will silently produce a broken child process invocation for manifests that omit the prefix.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Acknowledged. The manifest contract (per executor.go:47) stores TokenFlag WITH the -- prefix — both generators produce correct output for conforming manifests. The CLI generator's TrimPrefix is defensive normalization; the MCP generator trusts the contract.
The asymmetry is real but doesn't cause incorrect behavior for valid manifests. Adding normalization to the MCP generator would be a robustness improvement — tracking for a future pass.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| return fmt.Errorf("auth required: set {{$tokenEnv | esc}} or pass --{{$tokenFlag | esc}}") | ||
| } | ||
| _ = token // passed to the entrypoint via environment | ||
| cliArgs = append(cliArgs, "--{{$tokenFlag | esc}}", token) |
There was a problem hiding this comment.
Security: Token exposed in process argument list
Appending the token to cliArgs makes it visible in ps aux and /proc/<pid>/cmdline on Linux/macOS — readable by any local user.
Pass the token via the child process environment instead:
c.Env = append(os.Environ(), fmt.Sprintf("%s=%s", "{{$tokenEnv | esc}}", token))Then remove the cliArgs append for the token. The child process reads it back via os.Getenv("{{$tokenEnv}}"). This is the standard safe pattern for secrets passed to subprocesses.
There was a problem hiding this comment.
By design. Constitution rule 24 requires tokens be passed to entrypoints via CLI flags, matching the runner.BuildArgs() convention at executor.go:47. This is the established contract between Toolwright and entrypoint scripts — the entrypoint reads --token_flag value from argv, not from its environment.
The /proc/cmdline visibility is a known trade-off accepted in the architecture. Environment variables have their own exposure surface (/proc/environ, child process inheritance). The flag-based approach keeps the entrypoint contract explicit and testable.
internal/codegen/cli_go.go
Outdated
| return fmt.Errorf("auth required: set {{$tokenEnv | esc}} or pass --{{$tokenFlag | esc}}") | ||
| } | ||
| {{- end}} | ||
| if "{{$entrypoint | esc}}" == "" { |
There was a problem hiding this comment.
Code quality: This generates dead code for every non-empty entrypoint
The template bakes the entrypoint literal into the generated file, so for any non-empty entrypoint this produces a constant-expression guard like:
if "/usr/local/bin/run-job" == "" { // always false
return fmt.Errorf("run-job: entrypoint not configured")
}staticcheck (SA9003) will flag the unreachable branch. The clean fix is to validate the entrypoint in buildToolData at code-generation time and return a generation error — rather than emitting logic that can never execute.
There was a problem hiding this comment.
Fixed in 7c2a046. Entrypoint is now assigned to a local variable; guard and exec.CommandContext both use the variable. No constant-expression branches in generated output.
| {{- end}} | ||
| {{- if .HasAuth}} | ||
| // Auth token last (via CLI flag, constitution rule 24) | ||
| args.push("{{.TokenFlag | esc}}", envToken); |
There was a problem hiding this comment.
Security: Token exposed in process argument list (same issue as CLI generator)
args.push("--token", envToken) passes the secret as a child process argument, visible in ps aux on the host.
Pass it via environment instead:
const { stdout } = await execFile("{{.Entrypoint | esc}}", args, {
env: { ...process.env, ["{{.TokenEnv | esc}}"]: envToken },
});Remove the args.push for the token. The child process reads it from its own environment.
There was a problem hiding this comment.
Same as CLI — by design per constitution rule 24. The token-via-CLI-flag contract is the established interface between Toolwright and entrypoint scripts.
internal/codegen/mcp_typescript.go
Outdated
| } | ||
| {{- end}} | ||
| // Empty entrypoint guard | ||
| if (!"{{.Entrypoint | esc}}") { |
There was a problem hiding this comment.
Code quality: Constant-expression guard — always false at runtime
For any non-empty entrypoint this generates if (!"./health.sh") which is always false. ESLint and TypeScript strict mode will surface this as dead code.
Prefer validating tool.Entrypoint in buildTSToolData at generation time and returning a buildErr there.
There was a problem hiding this comment.
Fixed in 7c2a046. MCP template now uses const entrypoint = "..."; if (!entrypoint) — no constant-expression guard.
| } | ||
| parsed{{.GoName}}[i] = v | ||
| } | ||
| for _, v := range parsed{{.GoName}} { |
There was a problem hiding this comment.
Style: Unnecessary two-pass parse for int[] elements
This block parses each string element to int into parsedFoo, then immediately formats every element back to string for cliArgs. The intermediate []int slice is never used for anything else — the only effect is validation.
A single-pass loop that validates and appends in one step is simpler and avoids the allocation:
for _, s := range runJobFlagCounts {
if _, err := strconv.Atoi(s); err != nil {
return fmt.Errorf("invalid value %q for element of --counts: not a valid int", s)
}
cliArgs = append(cliArgs, "--counts", s)
}Same pattern applies to the float[] block below.
There was a problem hiding this comment.
Keeping the two-pass pattern. The round-trip (parse then format) normalizes values — e.g., "007" → 7 → "7". This is intentional: the entrypoint receives canonical representations, not raw user input. The intermediate slice also validates all elements before appending any to the args slice.
internal/codegen/cli_go.go
Outdated
| {{- else}} | ||
| // Execute the tool entrypoint. | ||
| c := exec.CommandContext(cmd.Context(), "echo", "running", "{{$toolName}}") | ||
| c := exec.CommandContext(ctx, "{{$entrypoint | esc}}", cliArgs...) |
There was a problem hiding this comment.
Review Summary
Good work replacing the echo/TODO stubs with real execution. The arg-building convention (positional → flags → token), type conversions, and test coverage are all solid. Two items need fixing before merge:
🔴 Security — Token in process arg list (this file line 891, mcp_typescript.go line 759): Both generators append the auth token to the args slice passed to the child process. On Linux/macOS ps aux and /proc/<pid>/cmdline make process arguments readable by any local user. Pass the token via c.Env / execFile env option instead.
🟡 Code quality — Constant-expression guards (this file line 781, mcp_typescript.go line 727): The entrypoint check bakes the literal path into generated code, producing an always-false branch for every non-empty entrypoint. staticcheck SA9003 / ESLint will flag this. Validate tool.Entrypoint at generation time in buildToolData/buildTSToolData and surface a generation error there.
🟡 Style — int[]/float[] round-trip parse (this file line 812): Validate-and-append in a single loop; no need for the intermediate parsed slice.
Everything else — test coverage (30 + 31 + 11 tests), flag ordering, esc on all interpolated strings, block scoping for array loops — is well done.
There was a problem hiding this comment.
Thanks for the thorough review. Summary of resolutions:
- Red (token in args): By design — constitution rule 24 requires CLI flags for token passing. This is the runner convention contract.
- Yellow (constant-expression guards): Fixed in 7c2a046 — both templates now use local variables.
- Yellow (two-pass parse): Keeping — the round-trip provides value normalization and validates-before-appending.
Also fixed the unused validateRequest import from the tool template (flagged by greptile) — it's HTTP middleware not applicable in MCP tool handlers.
…port - Use local variable for entrypoint in both CLI and MCP templates to eliminate constant-expression dead code (staticcheck SA9003 / ESLint) - Remove unused validateRequest import from MCP tool template — it's HTTP middleware not applicable in MCP tool handlers - Update 3 CLI test assertions to match variable-based pattern Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 participant
Summary
echostub with realexec.CommandContextcalls to manifest entrypoint scripts// TODO: implementstub withexecFilecalls using promisifiednode:child_processAcceptance Criteria
exec.CommandContextwith manifest entrypointescfunctiontoolGoDatastructexecFile/promisifyfrom node builtinsexecFilewith manifest entrypointtsToolData, escaped viaescGate Results
Aggregate: 0 BLOCK, 1 WARN, 21 INFO
Key Files
internal/codegen/cli_go.go— Entrypoint field, template rewrite (+113/-69)internal/codegen/cli_go_test.go— 30 new AC1/AC2/AC6 tests (+761)internal/codegen/mcp_typescript.go— Entrypoint/TokenFlag fields, template rewrite (+55/-10)internal/codegen/mcp_typescript_test.go— 31 new AC3/AC4/AC6 tests (+842)internal/codegen/integration_test.go— 11 new cross-generator tests (+331)🤖 Generated with Claude Code