feat: Add port field to Spec and support config file port override#93
feat: Add port field to Spec and support config file port override#93
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
main.go (1)
93-102: Validate resolved port before starting to avoid late failureAfter resolution, ensure
portis within 0..65535 and not-1anymore. This prevents passing nonsense intoRunSubgraph.Apply this diff:
return internal.RunSubgraph( ctx, cancel, - port, + func() int { + if port == -1 { + // Should not happen; defensive guard. + return 3000 + } + return port + }(), openBrowser, log.Default(), wf, taskNames, split, )Alternatively, validate right before the call and
return fmt.Errorf(...)on violation instead of injecting a fallback.
🧹 Nitpick comments (10)
Makefile (1)
7-7: Prefer installeddeadcodeovergo runto avoid repeated downloads and flaky CIRunning the tool via
go run ...@latestfetches and builds on every invocation, adding network latency and variability. Align with thegoimportspattern for determinism and speed.Apply this diff:
- go run golang.org/x/tools/cmd/deadcode@latest ./... + command -v deadcode || go install golang.org/x/tools/cmd/deadcode@latest + deadcode ./...If you want CI to fail on findings, we can also wire this to fail the build when
deadcodeoutputs anything (follow-up if desired).schema/workflow.schema.json (1)
357-360: Constrain and documentWorkflow.port(0 disables; enforce valid range)As-is, negative ports are accepted by the schema. The runtime logic doesn’t support negative values (other than the CLI sentinel), and docs say 0 disables and default is 3000. Tighten the schema to catch bad configs early and clarify semantics.
Apply this diff:
"port": { "type": "integer", - "title": "port" + "title": "port", + "description": "UI port. 0 disables the UI. Omit to use default (3000).", + "minimum": 0, + "maximum": 65535 },tasks.yaml (1)
1-1: LGTM: YAML-levelport: 3001example aligns with new precedenceThis demonstrates config-file override cleanly and should be picked up when the CLI flag isn’t set. Consider adding a second example or comment showing
port: 0to illustrate “disable UI” as well.main.go (1)
38-38: Flag help shows “default -1” which contradicts the description; consider a custom flag typeBecause the default value is
-1,flagwill print “(default -1)” while your description says “default 3000”. This is confusing. Optional improvement: implement a smallintFlagthat tracks whether it was set; keep the printed default at 3000 while still detecting explicit user input.Example (no diff):
type intFlag struct{ set bool; val int } func (f *intFlag) Set(s string) error { v, err := strconv.Atoi(s); if err != nil { return err }; f.val, f.set = v, true; return nil } func (f *intFlag) String() string { return strconv.Itoa(3000) } // shown in help // usage: var portFlag intFlag flag.Var(&portFlag, "p", "port to start UI on (default 3000, zero disables)") // after Parse: // if portFlag.set { port = portFlag.val } else { /* resolve from YAML or default */ }docs/reference/workflow-defs-workflow-properties-port.md (2)
7-8: Document what the port actually controls, default, and how to disable.Readers won’t know this integer is the Kit UI/server port, that 0 disables, or that the default is 3000. Add a short description so the semantics are discoverable without having to read the CLI docs.
Apply this diff to add a concise description:
- +The workflow-level TCP port used by Kit's local UI/server. + +- Default: 3000 (when omitted) +- Disable by setting to 0 +- CLI flag `--port` takes precedence over YAML
15-15: Constrain and surface valid range (align schema + docs).Recommend constraining the schema to acceptable port values and reflecting that here. This prevents negative values (other than the internal CLI sentinel) and ports > 65535.
Consider updating the JSON Schema, then re-generating docs:
{ "$defs": { "Workflow": { "properties": { "port": { "type": "integer", "description": "Workflow-level TCP port for Kit's local UI/server. 0 disables; omitted => 3000; CLI --port overrides YAML.", "minimum": 0, "maximum": 65535 } } } } }Once added, this page will automatically show the description and range. Want me to open a follow-up to wire this into workflow.schema.json and regenerate?
docs/reference/workflow-defs-workflow.md (2)
21-21: Improve the “Defined by” label (avoid “Untitled schema”).The table shows “Untitled schema” for the port definition. If you set a meaningful "title" on the port schema node, the generator will render that instead, improving clarity.
Suggested schema snippet:
"port": { "type": "integer", "title": "Workflow port", "description": "Workflow-level TCP port for Kit's UI/server. 0 disables; omitted => 3000; CLI --port overrides YAML.", "minimum": 0, "maximum": 65535 }
29-41: State default, disable behavior, and precedence in the port section.Add three bullets to make behavior explicit and consistent with the implementation (CLI > YAML > default 3000; 0 disables).
Apply this diff:
## port `port` * is optional * Type: `integer` ([port](workflow-defs-workflow-properties-port.md)) * cannot be null -* defined in: [Untitled schema](workflow-defs-workflow-properties-port.md "https://github.com/kitproj/kit/internal/types/workflow#/$defs/Workflow/properties/port") +* defined in: [Untitled schema](workflow-defs-workflow-properties-port.md "https://github.com/kitproj/kit/internal/types/workflow#/$defs/Workflow/properties/port") +* Default: 3000 (when omitted) +* Disable by setting to 0 +* Precedence: CLI `--port` > YAML `workflow.port` > defaultdocs/reference/workflow.md (2)
1059-1059: Match label/title for the definition (avoid “Untitled schema”).Same nit as the other page: set "title" on the port schema so this cell reads “Workflow port” instead of “Untitled schema”.
1067-1084: Clarify semantics: default, disable, precedence.Mirror the behavior here so users don’t have to infer from CLI help.
Apply this diff:
### port `port` * is optional * Type: `integer` ([port](workflow-defs-workflow-properties-port.md)) * cannot be null * defined in: [Untitled schema](workflow-defs-workflow-properties-port.md "https://github.com/kitproj/kit/internal/types/workflow#/$defs/Workflow/properties/port") +* Default: 3000 (when omitted) +* Disable by setting to 0 +* Precedence: CLI `--port` > YAML `workflow.port` > default + +Notes: +- Valid values: 0 or 1–65535 (0 means disabled).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
Makefile(2 hunks)docs/reference/workflow-defs-workflow-properties-port.md(1 hunks)docs/reference/workflow-defs-workflow.md(1 hunks)docs/reference/workflow.md(1 hunks)internal/types/spec.go(1 hunks)internal/types/writer_func.go(0 hunks)main.go(2 hunks)schema/workflow.schema.json(1 hunks)tasks.yaml(3 hunks)
💤 Files with no reviewable changes (1)
- internal/types/writer_func.go
🧰 Additional context used
🪛 LanguageTool
docs/reference/workflow-defs-workflow.md
[style] ~39-~39: To form a complete sentence, be sure to include a subject.
Context: ...defs-workflow-properties-port.md)) * cannot be null * defined in: [Untitled sche...
(MISSING_IT_THERE)
docs/reference/workflow-defs-workflow-properties-port.md
[grammar] ~9-~9: There might be a mistake here.
Context: ... | | :------------------ | :--------- | :--...
(QB_NEW_EN)
[grammar] ~10-~10: There might be a mistake here.
Context: ...-------------------------------------- | | Can be instantiated | No | Unk...
(QB_NEW_EN)
[grammar] ~15-~15: There might be a mistake here.
Context: ...kflow-defs-workflow-properties-port.md))
(QB_NEW_EN)
🔇 Additional comments (1)
docs/reference/workflow-defs-workflow-properties-port.md (1)
3-5: Verify that the schema anchor URL resolves in rendered docs.The raw anchor to internal/types/workflow with JSON Pointer is fine if your doc tool resolves it, but it can produce a 404 on GitHub. If this is meant to be clickable in published docs, ensure the site generator rewrites/serves this route.
Run this quick check after building docs to ensure the link is valid in the published output.
There was a problem hiding this comment.
Pull Request Overview
This PR adds port configuration support to the workflow system, allowing users to specify a port in the configuration file that can be overridden by CLI flags. The port resolution follows a priority order: CLI flag > workflow port > default 3000, with 0 explicitly disabling the port.
Key changes:
- Added
portfield to the Spec struct and JSON schema - Updated main.go to implement port resolution logic with proper precedence handling
- Added static analysis tooling to detect dead code during builds
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tasks.yaml | Example configuration showing port 3001 setting |
| schema/workflow.schema.json | JSON schema definition for the new port property |
| main.go | Port resolution logic implementation with CLI flag precedence |
| internal/types/writer_func.go | Removed unused writeFunc type (dead code cleanup) |
| internal/types/spec.go | Added Port field to Spec struct with documentation |
| docs/reference/workflow.md | Documentation for the new port property |
| docs/reference/workflow-defs-workflow.md | Updated workflow definition docs |
| docs/reference/workflow-defs-workflow-properties-port.md | New port property documentation |
| Makefile | Added deadcode static analysis tool |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Summary by CodeRabbit
New Features
Documentation
Chores
Configuration