Skip to content

T-1197: Show variant agent and warn on consolidation mismatch in orbit finalize#158

Merged
ArjenSchwarz merged 4 commits into
mainfrom
T-1197/finalize-show-verify
May 10, 2026
Merged

T-1197: Show variant agent and warn on consolidation mismatch in orbit finalize#158
ArjenSchwarz merged 4 commits into
mainfrom
T-1197/finalize-show-verify

Conversation

@ArjenSchwarz

Copy link
Copy Markdown
Owner

Summary

  • Show variant agent info (alias, type, model) in the orbit finalize confirmation preamble, with graceful fallback when fields are missing.
  • Warn when --variant disagrees with the most recent chosen_variant_id in consolidation-log.json; warning fires before the --force gate so it's visible in CI output.
  • Adds specs/finalize-show-verify/ (smolspec, tasks, decision log) and a top-level specs/OVERVIEW.md catalog of all feature specs.

Test plan

  • Unit tests cover all/partial/no agent fields, mismatch firing, matching entries producing no warning, missing/corrupt/empty logs, and the --force path
  • make test passes
  • make lint passes

Plan the enhancement to `orbit finalize` so it (a) shows the agent
(alias, type, model) used for the variant being finalized in the
confirmation preamble, and (b) warns when --variant disagrees with
the most recent chosen_variant_id recorded in consolidation-log.json.

The mismatch warning is informational: the user can still proceed via
the existing y/N prompt or --force. A missing or unreadable
consolidation log is treated as "no verification possible" and skipped
silently, since consolidation is optional.

Adds specs/finalize-show-verify/{smolspec.md,tasks.md,decision_log.md}
with 5 implementation tasks (distributed test coverage plus a
lint/test gate). No code changes in this commit.
Generate a chronological catalog of all feature specs under specs/,
with status (Done/In Progress/Planned/No Tasks) derived from each
spec's tasks.md, plus per-spec file listings linking to the
underlying documents.

Excludes specs/bugfixes/ (tracked separately) and the empty
specs/gear-mode and specs/general placeholder directories.

Includes the new finalize-show-verify entry from the previous
commit on this branch.
…n orbit finalize

Render the variant's agent alias, agent type, and model in the finalize
preamble, falling back to "Agent: unknown" when no fields are populated.
Build the parenthetical conditionally so missing fields produce a clean
string with no empty parens, dangling commas, or stray "model: " labels.

Read consolidation-log.json (when present) and warn before the confirmation
prompt when the requested --variant differs from the most recent
consolidation entry's chosen_variant_id. The lookup runs before the --force
gate so the warning is visible in CI output. Missing log, parse failure, or
empty Entries are treated as "no verification" and emit nothing.

Tests cover all/partial/no agent fields, mismatch firing, matching entries
producing no warning, missing/corrupt/empty logs, and the --force path.
…rify

Three-level explanation (beginner / intermediate / expert) of the
cmd/orbit/finalize.go changes plus a Completeness Assessment table
mapping each numbered requirement in smolspec.md to the code that
satisfies it. Generated as part of pre-push-review validation; all
nine requirements explained cleanly with no gaps.
@claude

claude Bot commented May 10, 2026

Copy link
Copy Markdown

Code Review — T-1197: Show variant agent and warn on consolidation mismatch in orbit finalize

Overview

This PR adds two informational improvements to orbit finalize:

  1. Displays the variant's agent alias, type, and model in the preamble before the "This will:" block.
  2. Reads consolidation-log.json and prints a warning when --variant disagrees with the most recent chosen_variant_id.

The change is purely additive, contained to cmd/orbit/finalize.go, and the test coverage is thorough. The spec documentation (decision_log.md, smolspec.md, implementation.md) is exceptionally detailed. Overall this is a clean, well-reasoned change — comments below are mostly minor.


Code Quality

formatVariantAgentInfo — correct and readable. The conditional-build approach (separate parens and parts slices) is clean. One minor note:

// finalize.go:35
fmt.Printf("%s\n", formatVariantAgentInfo(targetVariant))

This can be simplified to fmt.Println(formatVariantAgentInfo(targetVariant))Println already appends a newline, and avoids a redundant format verb.

printConsolidationMismatchWarning — logic is correct and matches all the smolspec requirements. The placement before the --force gate is the right call (validated by the test). One naming note: the local variable log shadows Go's standard log package. While log isn't imported in this file today, a future log.Printf for debug tracing would silently break. A name like consolidationLog or clog would be safer.

Nil safetyformatVariantAgentInfo takes a non-nil *variants.Variant and would panic on nil. The call site is safe because targetVariant == nil is checked just before, but marking the parameter expectation (or changing the signature to accept a value) would prevent future callers from being surprised.


Test Coverage

Good: Every smolspec requirement has a corresponding test case, including the --force path and corrupt-JSON path. The writeRawConsolidationLog helper for the corrupt fixture is a nice touch.

One gap — multi-entry log: There is no test case with two log entries where the first mismatches but the second matches (or vice versa). The requirement is "most recent entry only", but the test suite only uses single-entry logs. Adding a case like:

"only most recent entry is checked": {
    setupLog: func(t *testing.T, repoRoot, specName string) {
        writeConsolidationLog(t, repoRoot, specName, []consolidation.LogEntry{
            mismatchEntry, // older entry: ChosenVariantID=2
            matchingEntry, // newer entry: ChosenVariantID=1
        })
    },
    args:        []string{"--variant", "1"},
    wantWarning: false, // should not warn — most recent matches
},

…would pin the "last element wins" invariant explicitly.

Test duplication: TestFormatVariantAgentInfo and TestFinalizeCommand_AgentInfoPreamble share the identical 5-case matrix. The unit test on the helper is valuable for fast feedback; the integration test is valuable for verifying the preamble wiring. But duplicating the same cases in both means 10 test cases to maintain when the format changes. Consider whether the integration test can be trimmed to 1-2 representative cases (e.g., "all populated" and "all empty") and rely on the unit test for the full matrix.

Panic safety in runFinalizeCapture: os.Stdin is restored via t.Cleanup, which is correct. os.Stdout, however, is restored inline — if finalizeCommand panics, stdout stays redirected for the remainder of the test run and will swallow subsequent output. A matching t.Cleanup (registered before calling finalizeCommand) would make this robust:

t.Cleanup(func() { os.Stdout = originalStdout })

Architecture / Design

Acknowledged design debt — Logger.Latest(): The implementation notes correctly call out that log.Entries[len(log.Entries)-1] is open-coded in two places (consolidation/logger.go:137 and the new finalize.go helper). Deferring is reasonable for a smolspec. Worth a follow-up issue or TODO comment to prompt the next person touching the consolidation package.

Warning on stdout vs stderr: The implementation notes justify this (preamble consistency, CI log visibility). Reasonable for now, but worth flagging: if a CI consumer ever tries to parse orbit finalize stdout, warnings would be mixed into structured output. No action needed for this PR — just something to keep in mind if an --output json mode is ever added.

No breaking changes: The rebase path (finalize.go:127-148) is untouched. Exit codes are unchanged. The additive-only nature of the diff makes rollback simple.


Specs / Documentation

specs/OVERVIEW.md is a genuinely useful addition — a single catalog of all specs with dates and status is helpful for orientation. decision_log.md covers the warn-vs-hard-error and silent-on-missing-log decisions clearly, including alternatives considered. implementation.md is thorough, though at 117 lines it borders on over-engineering for a 50-line production change; that said, it's informational only and doesn't hurt.


Summary

Area Assessment
Correctness ✅ No bugs found
Test coverage ✅ Good — one missing multi-entry case
Conventions ✅ Follows existing patterns
Security ✅ No concerns
Performance ✅ Read is lock-free; single file read per finalize invocation

Suggestions (none blocking):

  1. fmt.Printf("%s\n", ...)fmt.Println(...) at finalize.go:35
  2. Rename log variable in printConsolidationMismatchWarning to avoid shadowing
  3. Add a multi-entry consolidation-log test case to pin "most recent entry only" behavior
  4. Register os.Stdout restoration via t.Cleanup in runFinalizeCapture
  5. Consider trimming the TestFinalizeCommand_AgentInfoPreamble matrix to avoid duplication with TestFormatVariantAgentInfo

@ArjenSchwarz ArjenSchwarz merged commit d179e63 into main May 10, 2026
2 checks passed
@ArjenSchwarz ArjenSchwarz deleted the T-1197/finalize-show-verify branch May 10, 2026 13:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant