allod/strategy
Watch
2
0
Fork
You've already forked strategy
 0
Code Issues 7 Pull requests 1 Projects Releases Packages Wiki Activity Actions

review: record named-token pass 8 #37

Closed
vnprc-agent wants to merge 0 commits from vnprc-agent/pi-named-tokens-pass-8 into master AGit
pull from: vnprc-agent/pi-named-tokens-pass-8
merge into: allod:master
Conversation 1 Commits - Files changed - +214
vnprc-agent commented 2026-08-22 16:15:23 +01:00
Contributor
Copy link

Findings: 1 BLOCKER, 0 GAP, 0 SIMPLIFY; 0 original-plan and 1 review-introduced.

The review-introduced BLOCKER in 5ed45c6 allowed an unconditional legacy-root sweep to delete unrecorded exact-target symlinks. Fixed in b6d95ec with bounded transitional ownership in the new manifest.

SIMPLIFY considered the unconditional scan, desired-set lookup, manifest-carried transitional ownership, a separate journal, conversion recovery, legacy fallback, startup flags, selector/provider separation, preference helpers, deployment scope, and quota/failover machinery; no further scope can be removed.

Model: gpt-5.6-sol

Findings: 1 BLOCKER, 0 GAP, 0 SIMPLIFY; 0 original-plan and 1 review-introduced. The review-introduced BLOCKER in 5ed45c6 allowed an unconditional legacy-root sweep to delete unrecorded exact-target symlinks. Fixed in b6d95ec with bounded transitional ownership in the new manifest. SIMPLIFY considered the unconditional scan, desired-set lookup, manifest-carried transitional ownership, a separate journal, conversion recovery, legacy fallback, startup flags, selector/provider separation, preference helpers, deployment scope, and quota/failover machinery; no further scope can be removed. Model: gpt-5.6-sol 
Review pass 1 findings on managed Pi named tokens:

[BLOCKER] Reconciler strictly requires ownership manifest .version==1 with
per-credential credentialLinks; per-token links change that shape, so the
first post-upgrade activation on a machine that already owns a provider would
refuse a valid prior-version manifest as corrupt and never converge. Plan now
requires the reconciler to recognize and upgrade a prior-version manifest in
place.

[GAP] Token-directory lifecycle was implicit: assert_recoverable_plan's
realpath -e parent check rejects a not-yet-existent <credential>/ dir,
safe_journal_path allows only single-segment *.age paths, and rollback/
recover/retire never remove the created directory. Plan now makes the token
directory a journaled transaction object that is created safely and pruned so
no empty/untracked directory survives.

[SIMPLIFY] rotationStrategy is stored/validated/tested but no code branches on
it, and provider-scoped rotate is being removed. Plan now drops it from the
record and migration; witnesses assert its absence.

Acceptance-test coverage extended for all three.
Pass 1 new findings: 1 BLOCKER, 1 GAP, 1 SIMPLIFY (3 total).
Classification: all 3 original-plan; 0 review-introduced.
Fixing hash: e04ccc0 (all three).

SIMPLIFY sweep considered: the migrate command (kept \u2014 one-shot forward-only
reader is safer than auto-migration), token list/default-none subcommands
(kept \u2014 required by the contract), and the rotationStrategy credential field
(removed \u2014 stored/validated/tested but no behavioral consumer once
provider-scoped rotate is dropped). No generic quota/failover machinery was
introduced.

Pass 1 confirmed via Pi 0.84.2 docs that session_start registerProvider,
agent_settled, and ctx.hasPendingMessages()/isIdle() exist, so the startup and
queued-work pinning claims (Focus Areas 1-2) are implementable \u2014 no findings
there. Because a BLOCKER was fixed, pass 2 is a scoped diff verification of
e04ccc0 by a different model.

Model: claude-opus-4-8
Clarify that legacy manifest records are strictly validated and used only to retire exact old links before desired per-token ownership is re-derived. Require rmdir-only token-directory pruning so concurrent or operator contents survive.

Findings: 0 BLOCKER, 2 GAP, 0 SIMPLIFY; both GAPs were introduced by review commit e04ccc0.
Pass 2 found 0 BLOCKER, 2 GAP, and 0 SIMPLIFY: 0 original-plan findings and 2 review-introduced findings. Both review-introduced GAPs are fixed in 9897fcd; metadata and pass-3 focus are updated here.

The SIMPLIFY sweep considered rotationStrategy, legacy fallback, startup flags, provider/token command separation, state helpers, deployment scope, and quota/failover machinery; no additional simplification was justified. Stop rule is not yet met.

Model: gpt-5.6
GAP: the plan never specified per-token runtime credential link naming.
Nested (mirroring the ciphertext layout) is required because both
credential and token names may contain hyphens, so a flat
<credential>-<token> link name cannot be split back into its parts for
per-credential token listing. Nesting under the credential name also
means a legacy single-file per-credential symlink and the new
per-credential token directory occupy the identical leaf path
(archetypes/modules/pi-provider-reconcile.sh: 'path=$credential_dir/$credential'),
so the manifest-version-transition fix from 9897fcd needs an explicit
retire-before-create ordering and a same-path collision refusal, or an
implementation could mkdir over the still-live legacy symlink and fail
the very upgrade path pass 1/2 fixed. Added the ordering guarantee and
a witness item for the same-path conversion.

Findings: 1 GAP, 0 BLOCKER, 0 SIMPLIFY. Original-plan (present since a6db27f,
missed by passes 1-2).
Findings: 1 GAP, 0 BLOCKER, 0 SIMPLIFY (original-plan, present since a6db27f).
0 review-introduced findings. Fixing hash: 31ae5c1.

SIMPLIFY sweep re-grounded rotationStrategy against the live Nexus script
(three read sites: stage_registries add-time check, credential-registry
schema exact()/enum check, print_impact's rotate in-place warning), all
reachable only through the provider-scoped add/rotate commands the plan
already replaces; pass 1's drop confirmed lossless, no further scope found
among legacy fallback, startup flags, command separation, state helpers,
deployment scope, or quota/failover machinery.

Stop rule not met: pass 3 produced an original-plan GAP, so neither
disjunct (review-introduced outnumbering originals, or neither
BLOCKER-nor-original-GAP for two consecutive passes) holds. Next pass is a
scoped diff verification of 31ae5c1.

Model: claude-sonnet-5
BLOCKER: the pass-3 same-leaf conversion required unlinking a legacy\nsymlink before creating its token directory, but the plan had only in-process\nrollback. A kill or reboot in that interval would leave neither object; the old\nstrict ownership validation would then refuse forever. Require a synced,\nbearer-free conversion journal that recovery consumes before manifest\nvalidation, restoring exact old ownership or preserving a foreign occupant.\n\nGAP: pin pending /token selections as process-local until activation succeeds,\nso /token cancel cannot accidentally leave a new remembered preference.\n\nFindings: 1 BLOCKER (review-introduced by 31ae5c1), 1 GAP (original-plan).
Findings: 1 BLOCKER (review-introduced by 31ae5c1), 1 GAP (original-plan).
Fixing hash: c7a5caa.

Pass 4 verified the same-leaf collision/order requirement but found its
SIGKILL/reboot gap. Pass 5 is a scoped blocker-fix review by a different model.

SIMPLIFY considered conversion-journal alternatives, preference helpers, legacy
fallback, startup flags, selector/provider separation, deployment scope, and
quota/failover machinery; no removable scope remains.

Model: gpt-5.6-terra
SIMPLIFY: the pass-3 same-leaf nesting put each credential's new token
directory at the exact legacy per-credential symlink leaf, forcing a
symlink-to-directory type change with an unavoidable absent-name interval,
which pass 4 then had to cover with a durable synced conversion journal,
recovery-before-manifest-validation, restore-exact-symlink, retire-before-create
ordering, and same-path collision refusal.

The credential-link path is an internal reconciler contract: the generated
auth.json !command is emitted against the same root in the same operation
(pi-provider-reconcile.sh line 352), so nothing external pins the legacy
location. Installing the new nested per-token links under a distinct root
that never contains a legacy leaf removes the type change entirely: create the
additive new links, commit auth/models/manifest atomically, then retire each
legacy symlink by exact recorded-target match. Every crash point is already
crash-safe with ordinary atomic renames and idempotent retry, so the whole
pass-3/pass-4 migration journal machinery is deleted while all contracts hold.

Findings: 1 SIMPLIFY (removes review-introduced machinery from 31ae5c1/c7a5caa).
Pass 5 scoped diff verification of c7a5caa held: the conversion journal is
internally sound for the same-leaf design (synced before unlink, consumed
before manifest validation, restores exact legacy symlink or preserves a
foreign occupant, empty-only rmdir, never recursive). The directed SIMPLIFY
sweep then dissolved that design.

New findings by tag: 1 SIMPLIFY, 0 BLOCKER, 0 GAP, 0 QUESTION.
Origin: the SIMPLIFY is review-introduced (removes machinery added by review
passes 3 (31ae5c1) and 4 (c7a5caa)); 0 original-plan findings.
Fixing hash: 362d3d0 (plan), this commit (prompt metadata).

SIMPLIFY sweep considered: same-leaf conversion + crash journal vs installing
the new nested per-token links under a distinct managed root (adopted \u2014 the
link path is an internal reconciler contract; auth.json !command is generated
against the same root in one operation, pi-provider-reconcile.sh line 352, so
nothing external pins the legacy leaf; distinct root removes the type change,
absent-name interval, journal, pre-unlink sync, restore-exact-symlink recovery,
and same-path collision handling while keeping unambiguous per-credential token
listing, forward-only migration, and operator-collision preservation); plus
rotationStrategy, legacy fallback, startup flags, selector/provider split,
preference-state helpers, deployment scope, and quota/failover machinery
(no further removable scope).

Startup (Focus 1) and queued-work pinning (Focus 2) re-confirmed against Pi
0.84.2 docs/extensions.md, docs/rpc.md, and docs/models.md. Pass-2 empty-only
rmdir and pass-4 preference-after-activation fixes remain intact.

Stop rule not met: neither disjunct has two consecutive qualifying passes
(pass 4 had a BLOCKER and a 1-vs-1 count). Next: scoped diff verification of
362d3d0 by a different model.

Model: claude-opus-4-8
New findings: 1 GAP, 0 BLOCKER, 0 SIMPLIFY; 1 original-plan, 0 review-introduced.
Fixed by 293362d. SIMPLIFY sweep found no removable scope.
Model: gpt-5.6-luna
Scoped diff review of 293362d. The migration ordering retires legacy
per-credential symlinks after the new-version manifest commit, but the
only prior source for which credentials had a legacy link was the old
manifest's credentialLinks list, and that list is gone once the new
manifest is committed. A credential retired in the same operation that
performs the version bump would never appear in any future desired-
credential set, so retirement keyed off "currently desired credentials"
could never find and clean up its legacy link, and a crash between the
manifest commit and retirement had no persisted list to resume from.

Fix: legacy retirement is a standing, credential-set-independent sweep
of the legacy link root's direct entries, retiring any entry that is a
symlink to its own name's computed runtime target regardless of current
desired status, run unconditionally on every activation rather than
only right after a version transition. This needs no persisted list of
which credentials used to have a legacy link, so it self-heals across a
crash between manifest commit and retirement and across simultaneous
credential removal. Added the combined migration+removal case to the
archetypes acceptance-test witness.
Pass 7 (claude-sonnet-5, reasoning medium): scoped diff verification
of 293362d. 1 original-plan GAP, 0 review-introduced. Legacy-link
retirement after the new-version manifest commit had no source for a
credential's legacy target once the old manifest was overwritten, so a
credential retired in the same operation as the version bump could
never be found by retirement keyed on the current desired-credential
set, and a crash between manifest commit and retirement had no
persisted list to resume from. Fixed in 5ed45c6 by making retirement
an unconditional, credential-set-independent sweep of the legacy link
root, self-healing from the directory alone.

Fresh sweep found no further BLOCKER/GAP; SIMPLIFY reconsidered the
sweep fix against a persisted pending-retirement list (rejected,
reintroduces journal-like bookkeeping) and re-confirmed prior SIMPLIFY
conclusions (conversion journals, pre-unlink sync, restore-symlink
recovery, same-leaf collision handling, legacy fallback, startup
flags, provider/token separation, preference helpers, deployment
scope, quota/failover machinery); no further removable scope found.

Stop rule not met: review-introduced findings (0) do not outnumber
original-plan findings (1), and this pass found an original-plan GAP.
Continue with a scoped diff verification of 5ed45c6 by a different
eligible model than claude-sonnet-5.

Model: claude-sonnet-5
Findings: 1 BLOCKER, 0 GAP, 0 SIMPLIFY; 0 original-plan and 1 review-introduced.

The review-introduced BLOCKER in 5ed45c6 allowed an unconditional legacy-root sweep to delete unrecorded exact-target symlinks. Fixed in b6d95ec with bounded transitional ownership in the new manifest.

SIMPLIFY considered the unconditional scan, desired-set lookup, manifest-carried transitional ownership, a separate journal, conversion recovery, legacy fallback, startup flags, selector/provider separation, preference helpers, deployment scope, and quota/failover machinery; no further scope can be removed.

Model: gpt-5.6-sol
vnprc-agent commented 2026-08-22 16:44:54 +01:00
Author
Contributor
Copy link

Superseded by #39, which contains the complete reviewed plan history.

Superseded by #39, which contains the complete reviewed plan history.
vnprc-agent closed this pull request 2026-08-22 16:44:56 +01:00

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
bug
Something is not working

  
duplicate
This issue or pull request already exists

  
enhancement
New feature

  
help wanted
Need some help

  
invalid
Something is wrong

  
question
More information is needed

  
wontfix
This won't be fixed

  
bug
Something is not working

  
duplicate
This issue or pull request already exists

  
enhancement
New feature

  
help wanted
Need some help

  
invalid
Something is wrong

  
question
More information is needed

  
wontfix
This won't be fixed

No labels
bug
duplicate
enhancement
help wanted
invalid
question
wontfix
bug
duplicate
enhancement
help wanted
invalid
question
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
allod/strategy!37
No description provided.