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 plan pass 11 #39

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

Pass 11 independently reviewed 72dbcff and completed a fresh sweep.

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

The pass-10 direct volatile-path simplification survives, but its manifest-before-unlink order discarded the sole legacy ownership proof across SIGKILL. fc6cfe5 keeps the strict v1 manifest until all recorded links are absent, permits absent-as-already-retired retry, and publishes the empty map last.

SIMPLIFY sweep considered transitional ownership maps, conversion journals, distinct runtime-link roots, manifest migrations, pre-unlink sync, SIGKILL restore machinery, runtime directory pruning, fallback/startup flags, selector separation, preference helpers, deploy scope, and quota/failover machinery; no further removable scope was found.

The consecutive count reaches the first stop disjunct, but the new blocker correction still requires the prompt-mandated different-model scoped verifier.

Model: gpt-5.6-sol

Pass 11 independently reviewed 72dbcff and completed a fresh sweep. Findings: 0 original-plan, 1 review-introduced; 1 BLOCKER, 0 GAP, 0 SIMPLIFY. Fix: fc6cfe5. The pass-10 direct volatile-path simplification survives, but its manifest-before-unlink order discarded the sole legacy ownership proof across SIGKILL. fc6cfe5 keeps the strict v1 manifest until all recorded links are absent, permits absent-as-already-retired retry, and publishes the empty map last. SIMPLIFY sweep considered transitional ownership maps, conversion journals, distinct runtime-link roots, manifest migrations, pre-unlink sync, SIGKILL restore machinery, runtime directory pruning, fallback/startup flags, selector separation, preference helpers, deploy scope, and quota/failover machinery; no further removable scope was found. The consecutive count reaches the first stop disjunct, but the new blocker correction still requires the prompt-mandated different-model scoped verifier. 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
Empty-only rmdir pruning applied to the Nexus secrets token directory and
the legacy runtime directory was never extended to the new nested
<token-link-root>/<credential>/ subdirectories the pass-3 design created.
Credential/last-token removal stranded empty directories, undermining
final-empty-state and partial-removal contracts. Require empty-only rmdir
pruning of emptied nested subdirectories and the token-link root, preserving
operator contents, and cover it in the archetypes witness.
Scoped diff verification of pass-8 BLOCKER fix (b6d95ec) plus fresh sweep.
Verifying model claude-opus-4-8, distinct from pass-8's gpt-5.6-sol.

New findings by tag: 1 GAP, 0 BLOCKER, 0 SIMPLIFY.
Classification: 1 original-plan (GAP), 0 review-introduced.

- GAP (original-plan, present since pass-3 31ae5c1): the plan pruned the
  Nexus secrets token directory and the legacy runtime directory with
  empty-only rmdir but never pruned the new runtime nested
  <token-link-root>/<credential>/ subdirectories, stranding empty
  directories on credential/last-token removal and undermining the
  final-empty-state and partial-removal contracts. Fixed in f917884.

Scoped diff verification of b6d95ec HELD: the live reconciler's v1
credentialLinks strictly records every managed legacy link with exact
path/target, so the bounded transitional legacyCredentialLinks map is the
complete and only ownership set; retirement reads only that durable map,
never a directory scan, so an unrecorded exact-target operator symlink
survives. Crash boundaries and Pi 0.84.2 registerProvider/session_start/
agent_settled surfaces re-confirmed.

SIMPLIFY sweep considered: the transitional legacyCredentialLinks map
(kept), directory-wide scan (rejected by pass 8), durable journal /
pre-unlink sync / restore-symlink recovery (superseded by distinct root),
same-leaf conversion, desired-set-keyed retirement, rotationStrategy,
legacy fallback, startup flags, selector/provider split, preference-state
helpers, deployment scope, and quota/failover machinery; nothing further
removable.

Stop rule not met: review-introduced (0) do not outnumber original-plan
(1), and this pass found an original-plan GAP.

Model: claude-opus-4-8
Decisive SIMPLIFY of the entire runtime-link design (pass 10).

The credential-link path is an internal reconciler contract: the generated
auth.json !command is emitted against the same root in the same operation, so
nothing external pins it (confirmed against pi-provider-reconcile.sh and Pi
0.84.2 docs/custom-provider.md, where !command executes a command for the whole
config value against any argument). Agenix already publishes each credential at
a fixed, non-secret volatile path (/run/agenix/pi-provider-credential-<cred>),
which the reconciler reads and validates directly; the managed symlink under
~/.config/pi/credentials is pure indirection.

For named tokens the volatile path nests as
<runtime-root>/pi-provider-token/<credential>/<token> (paths and names are
non-secret and never parsed back, so the flat-name hyphen ambiguity that forced
nested links never arises). Generated auth commands and the selector address
these fixed paths directly, so no managed runtime symlink layer exists.

This removes from the plan: the distinct token-link root, nested runtime
directories and their empty-only rmdir pruning, the manifest-version migration,
the bounded transitional legacyCredentialLinks map, and all the associated
conversion-journal / pre-unlink-sync / restore-exact-symlink crash machinery
accumulated across passes 3-9. The manifest stays at v1: the strictly validated
prior manifest is the complete and only ownership proof, and the existing
transaction retires every recorded legacy per-credential link (regardless of
current desired status) and records credentialLinks as empty. Empty
credentialLinks passes v1 validation vacuously (verified). The prior manifest
persists until the new one lands, so a crash re-drives the same bounded,
idempotent legacy-link retirement with no new journal. An unrecorded
operator-created legacy-root symlink survives even when name/target coincide.

Bearer handling is unchanged: bearer only to Pi's stdout credential pipe, never
in argv or generated files.
Decisive SIMPLIFY of the entire runtime-link design, plus fresh sweep.
Reviewed f917884/5622359; fix committed in 72dbcff.

New findings by tag: 1 SIMPLIFY, 0 BLOCKER, 0 GAP.
Classification: 0 original-plan, 1 review-introduced (removes machinery
accreted across passes 3-9).

- SIMPLIFY (review-introduced): the managed runtime credential-link layer is
  pure indirection. The credential-link path is an internal reconciler
  contract (auth.json !command emitted against $credential_dir in the same
  operation, nothing external pins it), and agenix already publishes each
  credential at a fixed non-secret volatile path the reconciler reads and
  validates directly. Generated auth and /token selector commands can address
  the fixed volatile per-token path <runtime-root>/pi-provider-token/<cred>/
  <token> directly (docs/custom-provider.md: !command runs for the whole value
  against any path; docs/models.md: no TTL/caching). This removes the distinct
  token-link root, nested runtime directories and their empty-only rmdir
  pruning (pass-9 fix), the manifest-version migration, the transitional
  legacyCredentialLinks map (pass 8), and all conversion-journal/pre-unlink-
  sync/restore-exact-symlink crash machinery (passes 4-6). Manifest stays v1;
  credentialLinks:{} passes v1 validation vacuously (verified); existing
  transaction retires recorded legacy links and records empty, crash-safe via
  prior-manifest persistence with no new journal. Operator legacy symlink
  survives on name/target coincidence. Bearer handling unchanged. Fixed 72dbcff.

Verification: confirmed nothing external pins the link path (desired_auth in
pi-provider-reconcile.sh), agenix ageSecrets fixed volatile path in
pi-provider-lifecycle.nix, empty credentialLinks passes v1 jq validation, and
Pi 0.84.2 !command/registerProvider/session_start/agent_settled surfaces.

SIMPLIFY sweep considered: the runtime link layer (removed), the Nexus
secrets-checkout token-directory journal (kept: real git-checkout transaction),
rotationStrategy, legacy fallback, startup flags, selector/provider split,
preference-state helpers, deployment scope, and quota/failover machinery; no
further removable scope.

Fix stability: supersedes 362d3d0 distinct-root, b6d95ec transitional map, and
293362d/f917884 runtime-link ordering and nested-directory pruning (all removed).
The pass-4 preference-persistence-after-activation fix and pass-1/2 Nexus
directory journal survive.

Stop rule: not met. Pass 10 qualifies for both disjuncts but pass 9 (0
review-introduced vs 1 original-plan GAP) qualifies for neither, so no
two-consecutive run holds. Next: scoped diff verification of 72dbcff by a
different, preferably cross-vendor model (e.g. gpt-5.6-sol) since this removes
crash machinery; pass 9 was also claude-opus-4-8.

Model: claude-opus-4-8
Pass 11 [BLOCKER], review-introduced by the pass-10 direct-path simplification.

Publishing the empty credentialLinks manifest before unlinking legacy links discards the only ownership proof. A SIGKILL in that interval leaves an owned link that no later activation may identify safely; the claim that the prior manifest re-drives cleanup is false once that manifest has already been replaced.

Make the existing transaction's safe order explicit: publish direct-path auth and models, retire only exact recorded legacy links, then publish the empty-link manifest. Until that final rename, absent recorded paths are accepted as already retired so a kill after any unlink resumes idempotently; foreign occupants still refuse and unrecorded lookalikes remain untouched. Ordinary trapped failures restore the snapshot. Extend the generated witness and rollback contract across every boundary.
Pass 11 independently reviewed 72dbcff and completed a fresh sweep.

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

The pass-10 direct volatile-path simplification survives, but its manifest-before-unlink order discarded the sole legacy ownership proof across SIGKILL. fc6cfe5 keeps the strict v1 manifest until all recorded links are absent, permits absent-as-already-retired retry, and publishes the empty map last.

SIMPLIFY sweep considered transitional ownership maps, conversion journals, distinct runtime-link roots, manifest migrations, pre-unlink sync, SIGKILL restore machinery, runtime directory pruning, fallback/startup flags, selector separation, preference helpers, deploy scope, and quota/failover machinery; no further removable scope was found.

The consecutive count reaches the first stop disjunct, but the new blocker correction still requires the prompt-mandated different-model scoped verifier.

Model: gpt-5.6-sol
Pass 11 findings remain 0 original-plan and 1 review-introduced: 1 BLOCKER, 0 GAP, 0 SIMPLIFY, fixed in fc6cfe5.

Passes 10 and 11 therefore satisfy the first two-consecutive-pass disjunct. Terminate this sequence; if review resumes, the blocker fix must receive a different-model scoped verification first.

SIMPLIFY sweep considered transitional maps, conversion journals, distinct runtime-link roots, manifest migrations, pre-unlink sync, SIGKILL restore machinery, runtime directory pruning, fallback/startup flags, selector separation, preference helpers, deploy scope, and quota/failover machinery; no further simplification was found.

Model: gpt-5.6-sol
vnprc-agent commented 2026-08-22 17:01:20 +01:00
Author
Contributor
Copy link

Opened unintentionally by the agent while direct master publication was expected to remain blocked. Closing immediately pending permission-boundary repair.

Opened unintentionally by the agent while direct master publication was expected to remain blocked. Closing immediately pending permission-boundary repair.
vnprc-agent closed this pull request 2026-08-22 17:01:21 +01:00
allod-agent commented 2026-08-22 17:15:26 +01:00
Member
Copy link

At the owner's request the two documents from this closed PR landed on master by fast-forward to 036680b, preserving the commit hashes the review prompt's pass history references. Next step per the review prompt stop-rule note: a scoped diff verification of fc6cfe5 by a model other than gpt-5.6-sol, recorded as pass 12, before implementation begins.

At the owner's request the two documents from this closed PR landed on master by fast-forward to 036680b, preserving the commit hashes the review prompt's pass history references. Next step per the review prompt stop-rule note: a scoped diff verification of fc6cfe5 by a model other than gpt-5.6-sol, recorded as pass 12, before implementation begins.

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
2 participants
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!39
No description provided.