Feat/skill surface polish#80
Merged
Merged
Conversation
Phase SK tasks 1 and 2 — tighten the capture surface so
ctx decision add and ctx learning add reject submissions
that lack body content.
Behavior:
- ctx decision add now requires --context, --rationale,
--consequence to be present and non-placeholder.
- ctx learning add now requires --context, --lesson,
--application to be present and non-placeholder.
- Placeholder values rejected case-insensitively after trim:
tbd, n/a, na, none, see chat, see above, see below,
pending, to be done, plus whitespace-only.
- Substring matches are NOT placeholders ("we deferred the
rationale as TBD originally" passes; "TBD" alone does not).
Layout follows the project conventions:
- internal/config/validate/ holds the placeholder constants.
- internal/err/cli/ gains FlagEmpty, FlagPlaceholder,
FlagUnregistered, MarkRequiredFailed; format strings in
errors.yaml under err.validation.*.
- internal/cli/add/core/validate/ wires required-flag
marking and the placeholder-rejection PreRunE wrapper.
The noun-level Cmd() in decision/cmd/add and
learning/cmd/add calls RequireBodyFlags with the noun's
three body flags.
Tests cover: each placeholder value, whitespace, substring
acceptance, missing-flag rejection, and PreRunE chaining.
Spec: specs/skill-surface-polish.md
Signed-off-by: Jose Alekhinne <jose@ctx.ist>
Phase SK tasks 3–7 — tighten the skill surface so the capture and design skills match the rigor of the sibling editorial pipeline. - /ctx-spec gains a --brief <path> flag. When supplied, the skill reads the file as authoritative and skips the interactive Q&A. Authority order when sources disagree: frozen docs > recorded DECISIONS > the brief > agent inference (labeled TBD). Light compression for clarity is allowed; new facts are not. - /ctx-plan always offers to save the debated brief to .context/briefs/<TS>-<slug>.md after the interview. The brief is the canonical handoff to /ctx-spec --brief. - /ctx-decision-add, /ctx-learning-add, /ctx-task-add, /ctx-convention-add gain an "Authority boundary (vs other skills)" section. Each lists the cross-promotions it refuses to perform silently (learning→decision, learning→convention, casual remark→task, one-off choice→convention, etc.) so promotion only happens on explicit user ask. - The "light compression for clarity is allowed; new facts are not" wording is now standardized across the four capture skills; the same wording lands in /ctx-handover when Phase KB ships. - docs/reference/skills.md documents the --brief contract in the /ctx-spec entry, including the authority order. Spec: specs/skill-surface-polish.md Signed-off-by: Jose Alekhinne <jose@ctx.ist>
…d errors Earlier f32c8fd wired RequireBodyFlags via panic; the first attempt at fixing it (in the prior amend of this commit) replaced the panic with `_ = c.MarkFlagRequired(name)` and `value, _ := cmd.Flags().GetString(name)` — both silent error discards. That violates the project's "handle every error" convention (existing `_ =` discards elsewhere in the codebase are tech debt, not authorisation to add more). This refactor takes the right approach: PreRunE is the single enforcement point. Cobra defaults string flags to "", so the empty-value check catches missing flags through the same code path as placeholder rejection. MarkFlagRequired is dropped entirely (its only contribution was a "(required)" help-text annotation, which is redundant when PreRunE already enforces). GetString's error is propagated, not swallowed. Changes from f32c8fd: - RequireBodyFlags returns nothing, calls no MarkFlagRequired, handles GetString error explicitly via `if getErr != nil`. - decision/cmd/add and learning/cmd/add stop panicking. - internal/err/cli loses FlagUnregistered and MarkRequiredFailed (unused after this change), plus their DescKeys and yaml entries. - Hook-driven spell corrections (generalising → generalizing, standardised → standardized) accepted in the same two Authority Boundary blocks. Spec: specs/skill-surface-polish.md Signed-off-by: Jose Alekhinne <jose@ctx.ist>
Standardise the author byline on thought-piece posts. Release recaps and first-person dev stories keep their existing byline. Signed-off-by: Jose Alekhinne <jose@ctx.ist>
The previous shape of validate.RequireBodyFlags(c, flags...)
silently wrapped the caller's c.PreRunE, saving the prior hook
and chaining to it. That is action-at-a-distance — no other
helper in the codebase mutates a passed-in cobra.Command's
hooks. The caller had no way to know their PreRunE was being
decorated without reading the helper.
Refactor:
- RequireBodyFlags → BodyFlags. Pure function:
validates each named flag's current value, returns an
error on the first failure, leaves the command untouched.
- decision/cmd/add and learning/cmd/add install their own
PreRunE explicitly:
c.PreRunE = func(cobraCmd *cobra.Command, _ []string) error {
return validate.BodyFlags(cobraCmd, ...)
}
The wiring is visible at the call site.
- Tests rewritten to call BodyFlags directly after parsing
flags, asserting on pure-function behaviour (acceptance,
placeholder rejection, missing-flag rejection via the empty
check, first-failure ordering).
Spec: specs/skill-surface-polish.md
Signed-off-by: Jose Alekhinne <jose@ctx.ist>
internal/validate/ already exists and its doc.go explicitly declares itself the anchor for "future non-path validators". A separate internal/cli/add/core/validate/ duplicated that philosophy with a tiny pure-function helper. Fold the helper into internal/validate/ and delete the duplicate. Changes: - internal/validate/bodyflags.go — new file with BodyFlags and RejectPlaceholder (identical to the deleted helpers). - internal/validate/bodyflags_test.go — tests moved over. - internal/validate/doc.go — adds a "CLI Body-Flag Validation" section alongside the existing path validators. - internal/cli/decision/cmd/add/cmd.go and internal/cli/learning/cmd/add/cmd.go — import internal/validate instead of the old path. - internal/cli/add/core/validate/ — deleted. Spec: specs/skill-surface-polish.md Signed-off-by: Jose Alekhinne <jose@ctx.ist>
Removes internal/validate.BodyFlags entirely. The two callers (decision add, learning add) now loop their body flags themselves in PreRunE and call validate.RejectPlaceholder per (flag, value) pair. The wiring is fully visible at the noun-level constructor and the validate package becomes uniformly string-consuming (matches Symlinks, RejectPlaceholder); the cobra.Command-taking outlier is gone. The RejectPlaceholder helper moves to its own file (rejectplaceholder.go / rejectplaceholder_test.go) per the single-helper-per-file convention used elsewhere in the project (internal/sanitize/truncate.go). The BodyFlags fixture and tests that needed a *cobra.Command go away with it; only the four RejectPlaceholder unit tests remain. Bundled in this commit because they share the same branch arc and the repo is mid-rebuild after a Go toolchain bump: * go.mod bumped 1.26.1 -> 1.26.3 to match the local toolchain (the 1.26.1 cached toolchain in ~/go/pkg/mod was corrupt). * SKILL.md (ctx-spec copilot integration) gains the --brief contract section that was already merged into the canonical /ctx-spec skill in 55acbd8 but missed the embedded asset. * Handover note from the prior session is preserved under .context/handovers/ following the existing precedent. Spec: specs/skill-surface-polish.md Signed-off-by: Jose Alekhinne <jose@ctx.ist>
Captures the localization gap surfaced after 0ccc1a8: the placeholder set used by RejectPlaceholder is hardcoded English constants, has no .ctxrc override hook, and uses locale-naive strings.ToLower that misses Turkish dotted/dotless I and German sharp s once any non-ASCII placeholder enters the set. The spec proposes three coordinated moves in one future commit: 1. Move shipped defaults into an embedded YAML asset (internal/assets/commands/vocab/placeholders.en.yaml — exact path subject to convention audit). 2. Add a .ctxrc placeholders: key with EXTEND semantics, modeled on rc.SessionPrefixes() but combining user list onto defaults rather than replacing — the dominant case in this codebase is "Tarzan Turkish" (EN+TR intermingled), so replace would surprise. 3. Replace strings.ToLower with golang.org/x/text/cases.Fold for proper Unicode case folding at both ingest and compare time. Ship en-only in v1; ctx has no locale-specific assets anywhere yet, so this establishes the structure without committing to a tr.yaml landing in the same change. Phase 0 task added with #prerequisite-for-locale-work tag so the work is identified as gating any future locale-specific phase. Spec: specs/placeholder-i18n.md Signed-off-by: Jose Alekhinne <jose@ctx.ist>
Two captures from session 3c81f71b that future contributors need: * DECISIONS.md: rc.Placeholders() will use EXTEND semantics (user list appended to defaults, case-folded de-duplicated), diverging from rc.SessionPrefixes() which uses REPLACE. The divergence is intentional — Tarzan Turkish (EN+TR intermingled) is the dominant case, so REPLACE would force users to re-list every English default to add one Turkish term and silently regress baseline coverage. * LEARNINGS.md: the "compile vs go tool version mismatch" error is caused by a corrupt cached toolchain in ~/go/pkg/mod/golang.org/ toolchain@v0.0.1-go<X>.<platform>/, NOT by the system Go install. Reinstalling Go does not fix it; deleting the cached dir or bumping the go.mod pin does. Spec: specs/placeholder-i18n.md Signed-off-by: Jose Alekhinne <jose@ctx.ist>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Feat/skill surface polish