Skip to content

chore: conform cli-common to the library profile; document keyring dependency cost#58

Merged
rianjs merged 2 commits into
mainfrom
chore/55-conformance-and-dep-surface
Jun 11, 2026
Merged

chore: conform cli-common to the library profile; document keyring dependency cost#58
rianjs merged 2 commits into
mainfrom
chore/55-conformance-and-dep-surface

Conversation

@rianjs

@rianjs rianjs commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

Implements #55 under the revised scope (see the plan and quantification comments on the issue): the keyring dependency finding is documented here with measured numbers — the upstream fix is committed separately in #57 — and cli-common is conformed to the new library-repo profile from #54.

Changes

  • ci.yml — both jobs now use go-version-file: go.mod (ci.md §3); the hardcoded '1.26' literal was the exact drift the rule bans.
  • Makefilecheck gains build, so a green local check predicts a green CI run (repo-layout.md §2.1). The monolithic build-test CI job is deliberately untouched (remains a catalogued ci.md §8 divergence).
  • AGENTS.md / CLAUDE.md — thin peer indexes per agent-implementation.md §2 (point to docs/development.md and docs/README.md; no cross-reference between them; local source-of-truth links since this repo is the standards home).
  • docs/development.md — repo-local facts: package map with the doc sections each package implements, make check, hermetic-test rules, manual-tag/release-train policy.
  • working-with-secrets.md §1.10 — "Known dependency cost" paragraph: byteness/keyring compiles its 1Password openers (→ wazero, jaeger) into every consumer; measured 2026-06-11 at keyring v1.9.3: 63 packages, ~10.6 MB attributable symbols in the shipped slck binary, no dead-code elimination. Remediation tracked in chore: upstream opt-out build tag to ByteNess/keyring for the 1Password backends #57.
  • output-and-rendering.md §10 — color verification pass result (downstream of docs: resolve standards-family contradictions, gaps, and stale references (review decisions applied) #54's isatty flip), recorded with audited SHAs.

Color audit evidence (W4)

Audited every color path — color-package imports plus a raw-ANSI-literal sweep (\x1b[, \033[, ) of non-test source — at these SHAs:

Repo SHA Color paths Forcing found
atlassian-cli (jtk+cfl) af0b0b0 shared/view/view.go only none — NoColor disables only
google-readonly d61bbc2 internal/view/view.go, root flag plumbing none — drops to termenv.Ascii on no-color
newrelic-cli ed6e6a4 internal/view/view.go none — NoColor disables only
slack-chat-api edf1aa0 no color paths at all n/a
salesforce-cli f62ccc8 internal/view/view.go none — sets color.NoColor = true on no-color

Zero raw-ANSI writers, zero color.NoColor = false / EnableColor / forced renderer profiles family-wide. Library TTY-detection defaults are in effect everywhere → conformant with the amended §8.

Also on the issue

  • Quantification + access correction posted as a comment (ByteNess/keyring is pull-only; "we control the fork" in the issue body was wrong — fix re-routed to chore: upstream opt-out build tag to ByteNess/keyring for the 1Password backends #57).
  • govulncheck on slack-chat-api: 0 reachable vulnerabilities; 13 module-level findings in required-but-uncalled modules (the predicted audit noise).
  • Post-merge: cut the next manual tag (v0.3.1 — docs + repo mechanics only, no exported-symbol changes, so no consumer release train needed); consumer repins opportunistic.

Closes #55

…pendency cost

- ci.yml: read the Go version from go.mod via go-version-file (ci.md $3)
  instead of a hardcoded literal that can drift
- Makefile: check now includes build, so a green local check predicts a
  green CI run (repo-layout.md $2.1)
- add AGENTS.md and CLAUDE.md as thin peer indexes and
  docs/development.md for repo-local facts (agent-implementation.md $2,
  repo-layout.md $2.1)
- working-with-secrets $1.10: document the measured dependency cost of
  byteness/keyring's unconditional 1Password imports (63 packages,
  ~10.6 MB, no DCE); remediation committed in #57
- output-and-rendering $10: record the family-wide color verification
  result — no CLI forces color onto non-TTY output (audited at pinned
  SHAs; slck has no color paths at all)

Closes #55
@rianjs

rianjs commented Jun 11, 2026

Copy link
Copy Markdown
Contributor Author

Findings

  • Minor: docs/output-and-rendering.md:242 says the audited CLIs are “conformant with §8,” but the immediately preceding bullet at docs/output-and-rendering.md:241 still records gro as missing the required --no-color flag, which is also part of §8. The audit supports conformance with §8’s isatty-gating requirement, not full §8 color conformance. Tighten the final sentence to avoid contradicting the retained gro divergence.

  • Minor: docs/development.md:41 scopes the release-train rule to exported credstore / statedir / cache symbols, but this repo also exports statedirtest, and consumers can depend on behavior as well as symbol shape. To keep the local guide aligned with the package map and the library profile, make this cover exported API or behavior changes across all exported cli-common packages, including statedirtest.

@rianjs

rianjs commented Jun 11, 2026

Copy link
Copy Markdown
Contributor Author

No test coverage concerns. Every file in this PR is docs, CI config, a Makefile tweak, or agent-context files — there is no testable behavior change. The CI workflow update (go-version-file), the Makefile build step addition, the two new agent entrypoints, and the documentation updates to working-with-secrets.md and output-and-rendering.md are all non-code changes. Nothing here warrants unit tests.

@monit-reviewer monit-reviewer left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated PR Review

Reviewed commit: 245f18f

Summary

No issues found.

2 PR discussion threads considered.


Completed in 2m 15s | $1.18 | sonnet | daemon 0.2.127 | Glorfindel
Field Value
Model sonnet
Reviewers hybrid-synthesis, documentation:docs-reviewer, harness-engineering:harness-architecture-reviewer, harness-engineering:harness-enforcement-reviewer, harness-engineering:harness-knowledge-reviewer
Engine claude · sonnet
Reviewed by pr-review-daemon · monit-pr-reviewer
Duration 2m 15s wall · 2m 12s compute (Reviewers: 1m 42s · Synthesis: 30s)
Cost $1.18 (estimated)
Tokens 210.2k in / 12.8k out
Turns 10

Per-workstream usage

Workstream Model In Out Cache read Cache create Cost
hybrid-synthesis sonnet 37.0k 1.2k 13.8k 23.2k (1h) $0.16
documentation:docs-reviewer sonnet 44.2k 5.4k 11.2k 33.0k (1h) $0.28
harness-engineering:harness-architecture-reviewer sonnet 41.4k 658 0 41.4k (1h) $0.26
harness-engineering:harness-enforcement-reviewer sonnet 43.6k 2.5k 11.2k 32.4k (1h) $0.24
harness-engineering:harness-knowledge-reviewer sonnet 44.0k 3.1k 11.2k 32.8k (1h) $0.25

Re-reviews only run when @monit-reviewer is re-requested as a reviewer — push as many commits as you need, then re-request when ready. PRs targeting branches other than main, master are skipped, even when @monit-reviewer is re-requested.

@rianjs

rianjs commented Jun 11, 2026

Copy link
Copy Markdown
Contributor Author

Findings

No findings.

The post-daemon changes stay within #55’s intended scope. The two prior issues are resolved: docs/development.md now covers exported API or behavior changes across all exported packages, including statedirtest, and output-and-rendering.md narrows the color audit conclusion to §8’s isatty-gating requirement while preserving the gro --no-color divergence.

@rianjs rianjs merged commit cc31b37 into main Jun 11, 2026
4 checks passed
@rianjs rianjs deleted the chore/55-conformance-and-dep-surface branch June 11, 2026 12:26
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.

chore: credstore dependency surface + cli-common self-conformance (standards-review level 2)

2 participants