feat(credstore): scaffold module + credential-ref grammar (INT-429)#2
Conversation
Stand up the cli-common Go module (go 1.24, stdlib-only) with CI matrix (ubuntu/macos/windows build-test + lint), golangci-lint config mirroring the atlassian-cli sibling, and a Makefile check gate. Implement the credential-ref grammar per the Secret-Handling Standard 1.3: ParseRef / FormatRef / EscapeRefSegment over the <service>/<profile> form, the 2.1 default-ref codification (DefaultProfile / DefaultRef), and the typed *RefError with errors.Is sentinels. Covered by table-driven, round-trip, and fuzz tests. Closes #1 [INT-429]
|
Blocker
Major None. Minor
Nit None. Code Quality Lens: the implementation stays inside INT-429: no OS-touching code, no Store/Open creep, no speculative |
TDD Coverage AssessmentVerdict: Coverage is strong. No blocking gaps. Two minor points worth noting. What's well-covered
Minor
Nit
No missing coverage for the stated scope (ref grammar + escape). OS/keyring backends are correctly marked out of scope. |
…ests - CI: run all steps under bash (Git Bash on windows-latest) so PowerShell no longer mangles -coverprofile=coverage.out into a bad package path (Windows build-test leg was failing). - Tidy gate (CI + Makefile): git status --porcelain so a future untracked go.sum is caught, not just modified tracked files. - RefError.Error: drop the unreachable default branch; exhaustive switch plus a documented trailing fallback for out-of-range Kind. - Tests: pin FormatRef empty-before-invalid and service-before-profile ordering; assert the error message actually names the ref. [INT-429]
|
Addressed review feedback in Codex
TDD assessment
Local: build, |
windows-latest checks out CRLF (Git autocrlf) while go mod tidy and gofmt emit LF, so the tidy gate saw go.mod as modified. Pin LF in the working tree on all platforms. [INT-429]
|
Blocker None. Confirmed cleared: latest head Major None. Minor None. Nit None. Code Quality Lens: the |
monit-reviewer
left a comment
monit-reviewer
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 1139434
Approved with 8 non-blocking suggestions below. Address at your discretion.
Summary
| Reviewer | Findings |
|---|---|
| harness-engineering:harness-architecture-reviewer | 3 |
| harness-engineering:harness-enforcement-reviewer | 2 |
| harness-engineering:harness-knowledge-reviewer | 1 |
| harness-engineering:harness-self-documenting-code-reviewer | 2 |
harness-engineering:harness-architecture-reviewer (3 findings)
💡 Suggestion - credstore/ref.go:148
FormatRef leaves RefError.Ref as the zero value ("") for empty-segment and invalid-char errors, while ParseRef always populates Ref with the full input string. Error() then produces output like 'empty service segment in credential ref ""' where the trailing quoted empty string is redundant and misleading — FormatRef takes two separate args, not a single ref string. Callers using errors.As to inspect Ref for diagnostics get an empty string. Consider setting Ref to the offending segment or documenting the intentional semantic difference from ParseRef.
💡 Suggestion - credstore/ref.go:175
EscapeRefSegment pre-allocates cap=len(raw) bytes, but each non-passthrough byte expands to 2 bytes ('_' → '__') or 4 bytes (others → '_xNN'). Inputs with many non-ASCII or special characters (e.g. email-derived or path-derived identifiers) will always reallocate. For a shared library hot path, sizing at 2*len(raw) or using strings.Builder with a reasonable growth factor would avoid the predictable reallocation.
💡 Suggestion - .golangci.yml:17
The revive linter is enabled but restricted to only the 'unused-parameter' rule, disabling the full default rule set (exported-functions-have-comments, error-return, var-naming, receiver-naming, etc.). For a shared library consumed by multiple CLIs, these structural rules carry higher value than for application code. The stability rationale in the comment is valid, but the trade-off disproportionately reduces coverage of the public API surface.
harness-engineering:harness-enforcement-reviewer (2 findings)
💡 Suggestion - .github/workflows/ci.yml:43
actions/checkout@v4, actions/setup-go@v5, and golangci/golangci-lint-action@v7 are pinned to mutable major-version tags rather than immutable commit SHAs. A compromised or force-pushed tag can silently replace the action binary. Standard hardening pins to the full SHA (e.g. actions/checkout@11bd719) with the tag as a comment.
💡 Suggestion - .golangci.yml:18
The linters.exclusions.rules block follows a golangci-lint v2 config shape, but in golangci-lint v2.0.x the canonical location for per-path linter exclusions may be issues.exclude-rules (unchanged from v1). If linters.exclusions is not recognized, the errcheck exclusion on test files is silently dropped, causing spurious lint failures on unhandled error returns in *_test.go files. Verify against golangci-lint v2.0.2 docs.
harness-engineering:harness-knowledge-reviewer (1 findings)
💡 Suggestion - credstore/ref.go:120
ParseRef and FormatRef have asymmetric validation ordering: ParseRef checks all structural concerns (slash count, both-segment emptiness) before any charset check; FormatRef interleaves per-segment (service empty → service charset → profile empty → profile charset). Result: ParseRef("bad.svc/") returns ErrRefEmpty (profile) while FormatRef("bad.svc", "") returns ErrRefInvalidChar (service). The asymmetry is only partially covered by tests and is undocumented, which could confuse maintainers extending either function. A comment on FormatRef explaining the deliberate ordering would reduce the risk of a future reorder silently breaking the pinned test cases.
harness-engineering:harness-self-documenting-code-reviewer (2 findings)
💡 Suggestion - credstore/ref.go:163
EscapeRefSegment("") silently returns "", which is not a valid ref segment and will be rejected downstream by FormatRef/ParseRef with ErrRefEmpty. EscapeRefSegment is the only API entry point that accepts invalid input without returning an error, giving callers no signal at the escape site. Consider adding a test that chains EscapeRefSegment("") directly into FormatRef to make the failure mode explicit in the test suite, or add a doc note on the required follow-up validation.
💡 Suggestion - credstore/ref.go:75
RefError.Is compares only Kind, so errors.Is(err, ErrRefEmpty) matches any RefErrorEmpty regardless of which segment (service vs profile) was empty. Callers who need to distinguish sub-cases must use errors.As and inspect Segment, but there is no test demonstrating or documenting that pattern. For a shared library, a single test showing the errors.As/Segment path would prevent callers from incorrectly relying solely on errors.Is for disambiguation.
5 info-level observations excluded. Run with --verbose to include.
4 PR discussion threads considered.
Completed in 3m 59s | $0.92 | sonnet | daemon 0.2.116 | 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, harness-engineering:harness-self-documenting-code-reviewer, security:security-code-auditor |
| Engine | claude · sonnet |
| Reviewed by | pr-review-daemon · monit-pr-reviewer |
| Duration | 3m 59s wall · 11m 14s compute (Reviewers: 2m 10s · Synthesis: 1m 46s) |
| Cost | $0.92 |
| Tokens | 112.2k in / 44.8k out |
| Turns | 7 |
Per-workstream usage
| Workstream | Model | In | Out | Cache read | Cache create | Cost |
|---|---|---|---|---|---|---|
| hybrid-synthesis | sonnet | 29.1k | 5.9k | 0 | 29.1k (1h) | $0.21 |
| documentation:docs-reviewer | haiku | 3.8k | 9.8k | 0 | 0 | $0.05 |
| harness-engineering:harness-architecture-reviewer | sonnet | 16.6k | 6.3k | 2.1k | 14.5k (1h) | $0.16 |
| harness-engineering:harness-enforcement-reviewer | sonnet | 16.6k | 7.2k | 2.1k | 14.5k (1h) | $0.17 |
| harness-engineering:harness-knowledge-reviewer | sonnet | 16.6k | 5.3k | 2.1k | 14.5k (1h) | $0.14 |
| harness-engineering:harness-self-documenting-code-reviewer | sonnet | 14.6k | 5.2k | 2.1k | 12.4k (1h) | $0.13 |
| security:security-code-auditor | haiku | 14.8k | 5.0k | 0 | 14.8k (1h) | $0.05 |
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.
| - name: Test | ||
| run: go test -v -race -coverprofile=coverage.out ./... | ||
|
|
||
| lint: |
There was a problem hiding this comment.
🔵 Low (harness-engineering:harness-enforcement-reviewer): actions/checkout@v4, actions/setup-go@v5, and golangci/golangci-lint-action@v7 are pinned to mutable major-version tags rather than immutable commit SHAs. A compromised or force-pushed tag can silently replace the action binary. Standard hardening pins to the full SHA (e.g. actions/checkout@11bd719) with the tag as a comment.
Reply to this thread when addressed.
There was a problem hiding this comment.
Acknowledged, deferring. Sibling Collective repos (e.g. atlassian-cli) pin actions to major-version tags by convention; diverging here alone is inconsistent. SHA-pinning is a worthwhile repo-wide hardening pass but out of scope for INT-429 (scaffold + ref grammar).
| settings: | ||
| revive: | ||
| # Explicit rule list so behavior is stable across golangci-lint versions. | ||
| rules: |
There was a problem hiding this comment.
🔵 Low (harness-engineering:harness-enforcement-reviewer): The linters.exclusions.rules block follows a golangci-lint v2 config shape, but in golangci-lint v2.0.x the canonical location for per-path linter exclusions may be issues.exclude-rules (unchanged from v1). If linters.exclusions is not recognized, the errcheck exclusion on test files is silently dropped, causing spurious lint failures on unhandled error returns in *_test.go files. Verify against golangci-lint v2.0.2 docs.
Reply to this thread when addressed.
There was a problem hiding this comment.
Verified — no change needed. This .golangci.yml is a verbatim copy of atlassian-cli/shared/.golangci.yml, which CI runs successfully on golangci-lint v2.0.2, and this PR's own lint job is green on v2.0.2 with the test-file errcheck exclusion in effect. The linters.exclusions shape is recognized in v2.
| return service, profile, nil | ||
| } | ||
|
|
||
| // FormatRef is the inverse of ParseRef: it validates both segments and |
There was a problem hiding this comment.
🔵 Low (harness-engineering:harness-architecture-reviewer): FormatRef leaves RefError.Ref as the zero value ("") for empty-segment and invalid-char errors, while ParseRef always populates Ref with the full input string. Error() then produces output like 'empty service segment in credential ref ""' where the trailing quoted empty string is redundant and misleading — FormatRef takes two separate args, not a single ref string. Callers using errors.As to inspect Ref for diagnostics get an empty string. Consider setting Ref to the offending segment or documenting the intentional semantic difference from ParseRef.
Reply to this thread when addressed.
There was a problem hiding this comment.
Fixed in dfa1a1d. Error() no longer appends the misleading 'in credential ref ""' when Ref is empty (the FormatRef path); it now reads 'empty segment'. ParseRef always populates Ref so its messages are unchanged.
| } | ||
| } | ||
| return true | ||
| } |
There was a problem hiding this comment.
🔵 Low (harness-engineering:harness-knowledge-reviewer): ParseRef and FormatRef have asymmetric validation ordering: ParseRef checks all structural concerns (slash count, both-segment emptiness) before any charset check; FormatRef interleaves per-segment (service empty → service charset → profile empty → profile charset). Result: ParseRef("bad.svc/") returns ErrRefEmpty (profile) while FormatRef("bad.svc", "") returns ErrRefInvalidChar (service). The asymmetry is only partially covered by tests and is undocumented, which could confuse maintainers extending either function. A comment on FormatRef explaining the deliberate ordering would reduce the risk of a future reorder silently breaking the pinned test cases.
Reply to this thread when addressed.
There was a problem hiding this comment.
Fixed in dfa1a1d. Added a doc comment on FormatRef explaining the deliberate per-segment order (service before profile, empty before charset) and why it differs from ParseRef's structural-first order, with a note that the order is test-pinned.
| } | ||
| if !validSegment(profile) { | ||
| return "", &RefError{Kind: RefErrorInvalidChar, Segment: "profile", Ref: profile} | ||
| } |
There was a problem hiding this comment.
🔵 Low (harness-engineering:harness-self-documenting-code-reviewer): EscapeRefSegment("") silently returns "", which is not a valid ref segment and will be rejected downstream by FormatRef/ParseRef with ErrRefEmpty. EscapeRefSegment is the only API entry point that accepts invalid input without returning an error, giving callers no signal at the escape site. Consider adding a test that chains EscapeRefSegment("") directly into FormatRef to make the failure mode explicit in the test suite, or add a doc note on the required follow-up validation.
Reply to this thread when addressed.
There was a problem hiding this comment.
Addressed in dfa1a1d. The godoc already documents EscapeRefSegment("")=="" and the deferred validation; added TestEscapeRefSegmentEmptyRejectedDownstream chaining EscapeRefSegment("") into FormatRef to make the ErrRefEmpty failure mode explicit in the suite.
| return fmt.Sprintf("credstore: invalid credential ref %q", e.Ref) | ||
| } | ||
|
|
||
| // Is reports whether target is a *RefError of the same Kind, so callers |
There was a problem hiding this comment.
🔵 Low (harness-engineering:harness-self-documenting-code-reviewer): RefError.Is compares only Kind, so errors.Is(err, ErrRefEmpty) matches any RefErrorEmpty regardless of which segment (service vs profile) was empty. Callers who need to distinguish sub-cases must use errors.As and inspect Segment, but there is no test demonstrating or documenting that pattern. For a shared library, a single test showing the errors.As/Segment path would prevent callers from incorrectly relying solely on errors.Is for disambiguation.
Reply to this thread when addressed.
There was a problem hiding this comment.
Already covered. TestRefErrorMessageHasNoSecretButNamesRef uses errors.As(&RefError) and asserts re.Segment=="profile", demonstrating the errors.As/Segment disambiguation pattern for callers who need to distinguish sub-cases.
|
|
||
| // EscapeRefSegment deterministically encodes an arbitrary string into the | ||
| // segment charset [A-Za-z0-9_-], for CLIs that derive a profile from a | ||
| // richer identifier such as an email address (standard §1.3). |
There was a problem hiding this comment.
🔵 Low (harness-engineering:harness-architecture-reviewer): EscapeRefSegment pre-allocates cap=len(raw) bytes, but each non-passthrough byte expands to 2 bytes ('_' → '__') or 4 bytes (others → '_xNN'). Inputs with many non-ASCII or special characters (e.g. email-derived or path-derived identifiers) will always reallocate. For a shared library hot path, sizing at 2*len(raw) or using strings.Builder with a reasonable growth factor would avoid the predictable reallocation.
Reply to this thread when addressed.
There was a problem hiding this comment.
Won't change, by rationale. EscapeRefSegment runs once per profile derivation at setup/init time, not on any request hot path; a 2*len pre-size trades a deviation for unmeasurable gain. Keeping the change set surgical and traceable to INT-429.
| - exhaustive | ||
| settings: | ||
| revive: | ||
| # Explicit rule list so behavior is stable across golangci-lint versions. |
There was a problem hiding this comment.
🔵 Low (harness-engineering:harness-architecture-reviewer): The revive linter is enabled but restricted to only the 'unused-parameter' rule, disabling the full default rule set (exported-functions-have-comments, error-return, var-naming, receiver-naming, etc.). For a shared library consumed by multiple CLIs, these structural rules carry higher value than for application code. The stability rationale in the comment is valid, but the trade-off disproportionately reduces coverage of the public API surface.
Reply to this thread when addressed.
There was a problem hiding this comment.
Won't change here. The revive-restricted-to-unused-parameter config is an established cross-repo convention copied verbatim from atlassian-cli/shared (stability across golangci-lint versions). Expanding the revive rule set for shared libraries is a reasonable idea but is a separate cross-repo decision, not INT-429 scope.
Address PR review (all low-severity):
- Error(): drop misleading 'in credential ref ""' for FormatRef empty-
segment errors where no full ref string exists.
- Document FormatRef's deliberate, test-pinned validation order and how
it differs from ParseRef.
- Add a test making the EscapeRefSegment("") -> FormatRef rejection
path explicit.
[INT-429]
2 participants
Closes #1 · Jira INT-429 (child of epic INT-310).
First discrete unit of the
cli-commonshared credential library, per the Open CLI Collective Secret-Handling Standardworking-with-secrets.md§2.1 (ref grammar §1.3).Scaffold
github.com/open-cli-collective/cli-common,go 1.24, stdlib-only (nogo.sum).build-test(ubuntu/macos/windows): tidy-drift check (git diff --exit-code -- go.mod go.sum),go build -v,go test -v -race -coverprofile;lintjob viagolangci-lint-action@v7v2.0.2..golangci.ymlmirroringatlassian-cli/shared;Makefilecheck: tidy lint testgate.main: 4 required checks, strict, 1 review, linear history, squash-only, delete-on-merge. No signed-commit rule (none configured; sibling does not enforce either).Ref handling (§1.3 / §2.1)
ParseRef/FormatRef/DefaultRef/EscapeRefSegmentover<service>/<profile>; charset[A-Za-z0-9_-].*RefError(Kind/Segment/Ref) witherrors.IssentinelsErrRefEmpty/ErrRefSegmentCount/ErrRefInvalidChar. Refs are non-secret (§1.2) so messages name the ref; no leak.EscapeRefSegment: deterministic, reversible-by-construction encoding into the charset;Unescapeintentionally deferred.Tests
Table-driven
ParseRef/FormatRef/DefaultRef/EscapeRefSegment, round-trip property,FuzzParseRef(no-panic + segment validity + round-trip invariant). 48 cases,-race, local 5s fuzz clean.Out of scope (later INT-310 units)
Backends,
Store/Open, bundle ops +SetBundleatomicity, Linux fail-closed classification, redaction & migration helpers,v0.1.0tag, nrq/slck Go-1.24 bump.Architect review
Codex (session reused for this work stream) converged at blockers=0 majors=0 minors=0 nits=0 after two revision rounds (ParseRef empty-vs-segment-count ordering, CI lint checkout/setup-go + go.sum-absent handling, DefaultRef codification, EscapeRefSegment empty-case).