docs(repo): resolve Tier-3 plan-readiness audit findings#118
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cc24338ecb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Sawmonabo
force-pushed
the
docs/tier-3-plan-003-audit
branch
2 times, most recently
from
2a41884 to
a413cfc
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a413cfc021
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Sawmonabo
force-pushed
the
docs/tier-3-plan-003-audit
branch
from
a413cfc to
0a9ddbc
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0a9ddbc698
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Sawmonabo
force-pushed
the
docs/tier-3-plan-003-audit
branch
from
0a9ddbc to
6249995
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6249995505
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Sawmonabo
force-pushed
the
docs/tier-3-plan-003-audit
branch
from
6249995 to
a52e16e
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a52e16e79d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
NS-15 — Tier 3 plan-readiness audit of Plan-003 (Runtime Node Attach). Swaps the audited working copy into the corpus (5 #### Tasks blocks, 29 tasks; runtime-node table-CREATE ownership corrected to Plan-003-owned) and lands the cross-cutting contract shapes the audit ratified: - api-payload-contracts.md: clientVersion (EventEnvelopeVersion brand) on RuntimeNodeAttachRequest; derived readOnly boolean on the response (orthogonal to NodeState); runtimenode.* method-name registry (Tier 3, per the canonical METHOD_NAME_FORMAT). - shared-postgres-schema.md: client_version column on runtime_node_attachments. - backlog.md: BL-131 (Plan-003 Phase-5 renderer automated coverage, V1.1, criterion-gated on the Plan-023 renderer test harness). - cross-plan-dependencies.md section 6: NS-15 -> completed; NS-16 promoted blocked -> ready; Plan-002 Phase-5/6 tracking correction (NS-28 + NS-29 nodes, edges, detail entries), discharging the NS-13b forward-reference. The Spec-003 heartbeat degraded->offline threshold (D3d) lands in a separate Spec-003 PR (distinct governance lifecycle). Plan-003 stays approved (D1); every amendment is additive / surface-forwarding / correctness-correction. B1-B6 + G1-G6 pass. Review-round resolutions (Codex + pre-merge advisor, a413cfc -> HEAD): - T3.2 attach is an upsert (INSERT ... ON CONFLICT (node_id, session_id) DO UPDATE): reconnect reactivates an offline row (Spec-003 L65/L90), a revoked row is refused (terminal per runtime-node-model.md), and a cross-session active attach is rejected by the new idx_node_attachments_active partial unique index. - New invariant I-003-5 (one active session per node in v1) backs that index; T1.3/T3.6/T3.7 resolve heartbeat/detach to the single active attachment by nodeId (no sessionId on the wire). - T3.4 uses the imported VERSION_FLOOR_EXCEEDED_CODE constant, not a re-spelled string literal. - runtimenode.heartbeat/detach get named z.null() no-content response schemas: result: null over JSON-RPC, and a null-payload HTTP 200 { result: { data: null } } envelope over tRPC; api-payload registry cells + T4.1 SDK contract note updated. - T3.2's two refusals project as typed CONFLICT (409) wire errors via T3.4's catch-arm + errorFormatter pattern (cross-session caught on SQLSTATE 23505); P9 reworded, P10 added, enumerated-test count 23->25. Round-2 re-review (0a9ddbc -> HEAD): - T3.1's control-plane migration is version 3 (0003-runtime-nodes.ts), not 0002: v2 is taken by Plan-002's 0002-session-invites, and the migration-runner.ts header pre-allocates v3+ to Plan-003. Append { version: 3, sql } to MIGRATIONS after v2 in ascending version order. Files now lists migration-runner.ts (extended); the test applies 0003 against a DB already migrated through 0002. Round-3 re-review (6249995 -> HEAD): - Corrected the tRPC void-mutation shape for heartbeat/detach. A tRPC resolver returning null surfaces as a normal HTTP 200 envelope { result: { data: null } } (control-plane uses the default transformer, no data.json wrapper), not a 204: the SDK's parseTrpcResult calls response.json() on every 2xx response, so an empty 204 body would throw SyntaxError. The z.null() schemas are unchanged; fixed 6 doc sites (api-payload-contracts.md 487/506/518/ 520/522 + plan T1.3 step). - Fixed I-003-5 "Why load-bearing": nodeId-only heartbeat/detach resolve via the idx_node_attachments_active partial UNIQUE(node_id) index, not a non-existent (node_id, session_id) index "filtered to active states". The composite idx_node_attachments_node is the upsert ON CONFLICT target, not the resolution path for nodeId-only reads. Round-4 re-review (a52e16e -> HEAD): - Aligned the idx_node_attachments_active schema comment with terminal revocation. The partial UNIQUE excludes both 'offline' and 'revoked' rows from active-uniqueness (index mechanics), but reattach eligibility is a T3.2 decision: an 'offline' row reconnects, a 'revoked' row is refused (terminal per T3.2/P10). The prior wording ("offline / revoked rows do not block a later (re)attach") read as license to reactivate revoked nodes. Verified T3.6/T3.7 step text defers nodeId-only resolution to I-003-5 (no F2-class re-description). Refs: Plan-003, ADR-018, BL-131 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sawmonabo
force-pushed
the
docs/tier-3-plan-003-audit
branch
from
a52e16e to
5f038f4
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Summary
NS-15 — Tier-3 plan-readiness audit of Plan-003 (Runtime Node Attach). Swaps the audited working copy into the corpus and lands the cross-cutting contract shapes the audit ratified. Plan-003 stays
approved— every amendment is additive / surface-forwarding / correctness-correction.Audit runbook:
docs/operations/plan-implementation-readiness-audit-runbook.md. This is the Tier-3 pass (Tier-2 = NS-12 merged; G6 satisfied).Decisions ratified (D1–D3)
approved— all amendments additive / surface-forwarding / correctness-correction; 0 status-flips (B6=0)EventEnvelopeVersion+client_versioncolumn · 3b derivedreadOnlyboolean · 3cruntimenode.*namespace · 3d → separate Spec-003 PR · 3e presence node-global (ratify, no diff)D1 — why "stays approved" (steel-manned)
Three amendments are literal flip-triggers under the runbook rule (new Cross-Plan Obligation / new Required-ADR / behavior-change). Surfaced rather than buried, because D1 is the one place the audit verdict can be overruled:
apps/desktop/src/renderer/is Plan-023-owned) + Spec-023 §Trust Stance. Plan-003 consumes thewindow.sidekicksbridge; it does not build it.Matches every prior tier audit (NS-13b / NS-24 / NS-25 / NS-26 stayed
approvedon additive Tasks-block amendments). If the rule is read strictly and CP/Required-ADR additions must flip regardless of framing: Plan-003 →review, and a re-review pass is owed before any Tier-4 code consumes it (Tier-4 is several tiers out — procedural cost, not schedule-critical).D2 — why Option C
One squash-commit = one governance lifecycle. The api-payload/Postgres edits are
docs/architecture/(canonical-when-merged, no draft→review→approved gate) and belong with the plan that drives them. The Spec-003 §Default-Behavior amendment re-discharges the Spec-Status Promotion Gate — a different lifecycle — so it ships as its own reviewable unit. Option A (fold Spec-003 in) conflates two lifecycles in one squash; Option B (defer all four) leaves Phase 3 unbuildable (T3.2/3.3/3.4 need the field, the read-only repr, and the namespace ratified now).D3 — contract shapes
3a —
clientVersiontype-binds to the existingEventEnvelopeVersionbrand (semver MAJOR.MINOR), not a parallel regex string. ADR-018 §Decision #3/#7 tiemin_client_versionto MAJOR envelope bumps, so the reported field is the envelope version the daemon speaks. Persisted asclient_version TEXT NOT NULLso the read-only verdict derives from a stored fact (survives a later floor raise) and the roster displays it without a live daemon round-trip. Caveat: the comparison is semver-aware (MAJOR.MINOR), not lexicographic ("10.0" > "9.0"fails under string-sort) — T3.3 owns the comparator; the wire field is just the branded value.3b —
readOnlyis a derived boolean on the response, orthogonal toNodeState. Liveness (NodeState) and permission (read-only) are orthogonal axes — a node can beonlineandreadOnly. Folding read-only intoNodeStateloses that. Derived (client_version < min_client_version) = single source of truth, no denormalized boolean that goes stale on a floor raise. No Postgresstate-CHECK change.3c —
runtimenode.*namespace. The obviousruntime_node.*form is regex-rejected byMETHOD_NAME_FORMAT=/^[a-z][a-z0-9]*(\.[a-z][a-z0-9]*)+$/(no underscores).runtimenodeis the concatenated form of the established domain noun (runtime_node_*tables,runtime_node.*events) — least-invented vs the generic/ambiguousnode.*.runtimenode.capabilityupdateis the first multi-word procedure in the system; lowercase run-on (2-segment) matches the uniformsession.*arity and is mechanically safe on the JSON-RPC side. Lands as a new Tier-3 subsection, not an injection into the Tier-1 tRPC/JSON-RPC registries.3e — presence cardinality ratified node-global (no diff):
runtime_node_presencePK =node_id; heartbeat reflects daemon-process liveness (one process → one health state); a node in N sessions has N attachment rows but one presence row; the roster derives per-session presence by joining attachments→presence onnode_id.Naming note (preempting the
clientVersionoverlap)RuntimeNodeAttachRequest.clientVersion(EventEnvelopeVersionbrand, Plan-003 control-plane attach, floor-governing) is intentionally distinct from the pre-existingDaemonHelloParams.clientVersion(free-formstring, Plan-007 local-IPC handshake). Same name at different layers — each internally consistent ("client" = the connecting party at that layer). The brand alignment withmin_client_version/client_versionis load-bearing per ADR-018 §Decision #3/#7; renaming would diverge from the canonical floor-name chain.File manifest
docs/plans/003-runtime-node-attach.md#### Tasksblocks (29 tasks: T1.1–7, T2.1–6, T3.1–8, T4.1–4, T5.1–4); runtime-node table-CREATE ownership corrected to Plan-003-owned; W2 Precondition checked; Required-ADRs row; CP-003-3; Plan-007-partial dependency; cite fixesdocs/architecture/contracts/api-payload-contracts.mdclientVersion: EventEnvelopeVersiononRuntimeNodeAttachRequest· 3breadOnly: booleanon the response · 3cruntimenode.*Tier-3 method-name registrydocs/architecture/schemas/shared-postgres-schema.mdclient_version TEXT NOT NULLonruntime_node_attachmentsdocs/backlog.mddocs/architecture/cross-plan-dependencies.mdTag on merge:
plan-readiness-audit-tier-3-complete.Separate follow-up PR (not this one): Spec-003 §Default Behavior — heartbeat degraded→offline threshold + sweep owner (proposed: degraded after 2 missed beats/30s, offline after 4/60s, backstop sweep every 30s — that PR decides). Re-discharges the Spec-Status Promotion Gate. T3.6 forward-references it and invents no values.
§6 fold (cross-plan-dependencies.md)
:::ready→:::completed; PRs checkbox[x] tier-3.blocked→ready(Tier-3 upstream now complete).:::ready, grounded in PR feat(client-sdk): Plan-002 P5 membership client SDK #117 open/CLEAN as of 2026-05-26) and NS-29 (Plan-002 Phase 6 — session-members renderer,:::blockedbehind NS-28), withNS26 → NS28 → NS29edges + detail entries.Findings ledger — 31 findings, 7 critical-instances → 4 root causes
runtimenode.*7 phase-instances → 4 roots is the healthy re-flag pattern (each consuming phase independently surfaced the same upstream gap from its own vantage), not over-flagging. Majors (8): read-only-repr + tRPC/router (fold into roots above) · presence cardinality (3e, ratify) · no-automated-renderer-test (→ BL-131, V1.1-deferred with a named criterion) · Phase-1/2 cite/ADR/ownership (Tier-A, in the swap). No orphan majors.
DT-003-4(table-ownership) resolved in the working copy.Calibration + gates
B1–B6: B1 avg-critical/phase 1.4 (target 0–2; max-phase 3 ≤ 4) ✓ · B2 total 31 (5–50) ✓ · B3 Tasks:blocking 7.25:1 root / 4.1:1 instance (≥2:1) ✓ · B4 user-review ~45–75 min (30 min–2 hr) ✓ · B5 2 substantive advisor calls + step-10 (≥1, ≤5) ✓ · B6 0 status-flips (0–1) ✓
G1–G6: G1 skeleton additions-only (CP-003-3 + 5 Tasks subsections; 0 deleted headers) ✓ · G2 all 4 root criticals dispositioned (3 here, 1 → Spec-003 PR) ✓ · G3 ex-Tasks diff 1.03× (<1.5×); max 8 step-entries/Phase ≪ 50 ✓ · G4 29/29 Tasks Steps → Spec coverage + Verifies-invariant ✓ · G5 0 new
PR #Nrefs in the audited plan ✓ · G6 Tier-2 committed (NS-12 merged) ✓ · no-ephemeral-tags: 0F-003-*/DT-003-*/PR #Ntags in any committed file ✓Test plan
pnpm install --frozen-lockfileprovisioned the worktree; all pre-commit + commit-msg hooks green (gitleaks no leaks; lychee 259 links / 241 OK / 0 errors / 18 excluded; docs-corpus governance cite-walk; lint-staged prettier; commitlint).F-003-*/DT-003-*/PR #Ntags in any committed file (no-ephemeral-tags discipline).NS16node, no dangling edges; ready/blocked sets consistent with the edge set.clientVersionnaming overlap withDaemonHelloParamsverified non-conflicting (different interfaces / different layers).Refs: Plan-003, ADR-018, BL-131
🤖 Generated with Claude Code
Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com