Skip to content

docs(state): finalize §6 step 6 + tier-2 deferral for INT-310 tag-before-close [MON-5375]#22

Merged
rianjs merged 4 commits into
mainfrom
ktlo/MON-5375-int310-tag-before-close
May 20, 2026
Merged

docs(state): finalize §6 step 6 + tier-2 deferral for INT-310 tag-before-close [MON-5375]#22
rianjs merged 4 commits into
mainfrom
ktlo/MON-5375-int310-tag-before-close

Conversation

@rianjs
Copy link
Copy Markdown
Contributor

@rianjs rianjs commented May 20, 2026

Summary

INT-310 state-workstream close-out: doc-only PR that finalizes §6 step 6 with the port retrospective + codifies the 10 reusable patterns + reaffirms §5b tier-2 deferral.

After this merges, the tag-cut goes against HEAD of main:

git tag -a v0.1.0 -m "INT-310 state layer first stable: statedir + statedirtest + cache (tier 1) + credstore; per working-with-state.md §6 step 6"
git push origin v0.1.0

Then the 4-consumer repin train: atlassian-cli (3 modules: tools/cfl, tools/jtk, shared/) → google-readonly → slack-chat-api → newrelic-cli. sfdc has no cli-common dependency today; it will adopt v0.1.0 when un-parked (MON-5374).

Test plan

  • go test ./... — 252 passed (doc-only; nothing should change behaviorally)
  • CI green

Closes #19

…ore-close [MON-5375]

Adds §6.6 close-out retrospective: 5 of 6 port units shipped (jtk-cache,
Atlassian shared config, gro, slck, nrq); sfdc parked and explicitly
excluded from the v0.1.0 train (no cli-common dependency today).

Commons API additions during the rollout: cache.WriteEnvelope (#20) and
underscore in the cache-component regex (#21). Both additive; no
breaking change required across 5 ports.

Codifies the ten reusable patterns that emerged across the matrix:
mutation-free Detect/Apply split, Load/LoadForRuntime + cfg!=nil
contract, unexported xxxFromNewDir testable seam, malformed-old fails
loud before CopyNeeded, companion-plaintext-file dual-probe with
parsed-projection equality, 7-var statedirtest.Hermetic, the cleanup-
command recovery contract (cleanup must not be blocked by the broken
state it exists to wipe), init-gate ordering proof, reflect.DeepEqual
on default-applied Config, path-identity dedup.

Reaffirms §5b tier-2 deferral: only jtk needs the registry/DAG/fetcher
shape today (gro uses tier-1 only; cfl has no cache). Promoting from
one consumer would just relocate jtk code. Re-evaluate when cfl gains
a cache.

Closes #19
@rianjs
Copy link
Copy Markdown
Contributor Author

rianjs commented May 20, 2026

Findings

Major: Atlassian PR numbers are wrong in the retrospective table.
The SHAs match the shipped port commits, but the PR numbers do not:

  • atlassian-cli@59a2fb9 is #373, not #371
  • atlassian-cli@b867c0e is #374, not #372

The other rows match local history: gro #134, slck #165, nrq #101.

Major: Pattern 5 overstates the shipped equality model for companion secret files.
parsed-projection equality is correct for nrq’s key=value credentials file, but gro’s token.json migration compares the trimmed raw serialized token string and its test explicitly describes “different bytes” as divergent. The reusable rule should be phrased as: “compare the effective parsed/projection value when the format supports harmless representation differences; opaque token blobs may use raw value equality.”

Same bullet also names CredentialFileCandidates as if it were universal. That is nrq’s shape; gro used GetTokenPath + OldHandRolledTokenPath. Better to describe the generic pattern as old/new candidate enumeration with path-identity dedup, not one specific helper trio.

Major: Pattern 9 is too broad as a family rule.
reflect.DeepEqual on the full default-applied struct is what slck/nrq shipped, but gro needed a user-meaningful projection: CredentialRef, Keyring, sorted GrantedScopes, and OAuthClientPath equivalence for default old/new locations. A future port following the current wording would wrongly fail legitimate default-path relocations. Rephrase to: compare the full default-applied user-meaningful projection; use whole-struct DeepEqual only when every field is directly comparable after defaults.

Minor: Tier-2 deferral wording still says “third consumer.”
The criteria are “≥2 consumers need the same registry/DAG shape.” Since jtk is currently the only tier-2-shape consumer, the next qualifying consumer would be the second, not the third. Change “any third consumer needs the same shape” to “any second tier-2-shape consumer needs the same shape.”

- Atlassian PR numbers: #371→#373 (MON-5369 jtk-cache), #372→#374
  (MON-5370 shared config). SHAs were already correct.
- Pattern 5 (companion-secret-file dual-probe): format-dependent
  equality, not a single rule — text key=value uses parsed projection
  (nrq); opaque blobs use trimmed raw serialized value (gro's
  token.json). Generic shape described as old/new candidate
  enumeration + path-identity dedup, not nrq's specific helper trio.
- Pattern 9 (material equality): use whole-struct DeepEqual on
  default-applied Config ONLY when every field is directly comparable
  and user-meaningful (slck/nrq); build an explicit projection when
  fields need semantic normalization (gro: OAuthClientPath default
  equivalence, sorted GrantedScopes).
- §5b tier-2 reaffirmation: "second tier-2-shape consumer," not
  "third" (the criteria are ≥2 consumers).
@rianjs
Copy link
Copy Markdown
Contributor Author

rianjs commented May 20, 2026

Findings

Major: Pattern 5 still overstates the “same helper as clear --all” rule across both cited ports.
The format-dependent equality fix is good, but this sentence is still not accurate for gro:

“the shared candidate helper must be the SAME one clear --all consumes”

That is true for nrq’s CredentialFileCandidates() shape, where migrator and clear --all both use the same credential-file candidate list. It is not what gro shipped: gro’s token dual-probe is in the keychain migrator via GetTokenPath() + OldHandRolledTokenPath(), while config clear --all removes config/cache and does not consume that token candidate helper directly. Rephrase this part as a conditional family rule:

If cleanup also scrubs the companion file, migrator and cleanup must share the same old/new candidate enumeration; nrq’s CredentialFileCandidates() is the exemplar. gro’s token case used old/new enumeration in the migrator and excluded token.json from config relocation.

Nit: Pattern 9 has a typo: “silent silence.”
Suggested ending: “Either way, future fields force a deliberate choice; silence is what hides divergence.”

Everything else from r1 is addressed: table PR numbers now match local history, tier-2 wording matches the §5b criteria, and the equality section now correctly distinguishes gro from nrq.

@rianjs
Copy link
Copy Markdown
Contributor Author

rianjs commented May 20, 2026

Findings

Nit: Pattern 5’s gro wording is almost right, but “gro’s cleanup scope is config + cache only” is slightly imprecise.
gro config clear --all also clears the keyring token; what it does not scrub is the legacy token.json file candidate list. I’d narrow that sentence to “gro’s --all file-scrub scope is config + cache only; OAuth token cleanup happens through the keychain path, not the legacy token.json candidate enumeration.”

Everything else looks aligned now: PR numbers are correct, tier-2 wording matches the promotion criteria, pattern 5’s equality model is accurate, and pattern 9 now captures gro vs slck/nrq correctly.

Copy link
Copy Markdown

@monit-reviewer monit-reviewer left a comment

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: 60d0b66

Summary

No issues found. (2 info-level observations excluded)

3 PR discussion threads considered.


Completed in 1m 27s | $0.24 | sonnet | daemon 0.2.120 | 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 1m 27s wall · 1m 49s compute (Reviewers: 1m 06s · Synthesis: 19s)
Cost $0.24
Tokens 66.8k in / 8.2k out
Turns 5

Per-workstream usage

Workstream Model In Out Cache read Cache create Cost
hybrid-synthesis sonnet 25.4k 668 13.6k 11.8k (1h) $0.06
documentation:docs-reviewer haiku 6.0k 6.7k 0 6.0k (1h) $0.05
harness-engineering:harness-architecture-reviewer sonnet 11.8k 431 2.1k 9.7k (1h) $0.05
harness-engineering:harness-enforcement-reviewer sonnet 11.8k 76 2.1k 9.7k (1h) $0.04
harness-engineering:harness-knowledge-reviewer sonnet 11.8k 401 2.1k 9.7k (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.

@rianjs rianjs merged commit e48c73e into main May 20, 2026
4 checks passed
@rianjs rianjs deleted the ktlo/MON-5375-int310-tag-before-close branch May 20, 2026 11:58
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.

INT-310 tag-before-close: cut cli-common semver tag + repin all consumers + finalize working-with-state §6 / tier-2 call

2 participants