Skip to content

fix: align todos-plan with deployment doctrine - #87

Merged
andrei-hasna merged 1 commit into
mainfrom
fix/2ba4d4c0-todos-plan-doctrine
Aug 9, 2026
Merged

fix: align todos-plan with deployment doctrine#87
andrei-hasna merged 1 commit into
mainfrom
fix/2ba4d4c0-todos-plan-doctrine

Conversation

@andrei-hasna

@andrei-hasna andrei-hasna commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

Summary

  • replace the retired deployment classification in the canonical todos-plan skill with independent product-story, server-backend, authenticated-authority, and provider-role facts
  • preserve the full stable UUID, bounded collection, fresh CAS, protected opaque input, named C1-C9, immutable receipt, compensation, and fail-closed production envelope
  • add Codewith to the package-owned agent sync route and allow an explicitly named bundled skill to repair a missing portable-corpus entry

Provenance

The detailed active Codewith copy was manually synchronized on June 30, 2026 (Conversations messages 33579 and 33583; Todos plan 4bc933b6-8b0a-4e38-8807-aa11b034a9af). It had no .hasna-skills.json ownership marker and its detailed content was absent from reachable repository history, so the active copy had diverged without a source-owned update route.

Regression evidence

Red before implementation:

  • bun test src/lib/todos-plan-contract.test.ts: 1 pass, 2 fail
  • failures proved the canonical skill lacked the required production authority envelope and the named Codewith render route could not resolve bundled-only source

Green candidate:

  • python3 .../quick_validate.py skills/todos-plan: Skill is valid!
  • focused tests: 54 pass, 0 fail, 206 assertions
  • bun run typecheck: pass
  • bun run build: pass
  • full bun test: 1076 pass, 0 fail, 6628 assertions
  • bun run verify:release: pass
  • isolated built-CLI Codewith render: rc=0, source and installed SHA-256 both 5513685f72f3c305481d69be0e936dca57f1a76c93f51b092fee918f54a224b5, management marker present
  • staged secrets scan: 5 files scanned, 0 findings, not truncated

Scope boundary

This PR does not merge, publish, install, edit profiles, or change the active ~/.codewith/skills/todos-plan copy. Independent review and installed rollout remain with the parent coordinator.

Todos task: 2ba4d4c0-89dd-4525-b4dc-fbc8331e9383


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

@andrei-hasna
andrei-hasna merged commit b1135a2 into main Aug 9, 2026
2 checks passed
@andrei-hasna

Copy link
Copy Markdown
Contributor Author

[REVIEW] NO_GO — #87 @ 5444e5a — lens: correctness+security+gates, reviewer Trebius (1 of 1)

Reviewed the exact PR base 5520033a662763ec1ce1353363d9e503e76e74d1 through head 5444e5a38876ad91220c3005cdbaac850cd5e81a. I read the full diff for all five changed files and the surrounding sync implementation, portable-skill name normalization and manifest loading, CLI sync argument flow, and existing sync tests.

Commands run, unpiped:

  • bun install — exit 0. Setup only; 179 packages installed.
  • bun run typecheck — exit 0. TypeScript emitted no diagnostics; this gate has no test pass/fail count.
  • bun run test — exit 0. 1075 pass, 1 skip, 0 fail; 6626 expect calls across 1076 tests and 85 files.
  • Malformed-name abuse probe against syncSkillsToAgents — exit 0 and printed {"action":{"skill":"../../skills/todos-plan","agent":"codewith","path":"<probe-home>/skills/todos-plan/SKILL.md","action":"create"},"escapedExists":true,"intendedExists":false}.
  • Valid-name control for the same bundled fallback — exit 0 and printed {"action":{"skill":"todos-plan","agent":"codewith","path":"<probe-home>/.codewith/skills/todos-plan/SKILL.md","action":"create"},"intendedExists":true,"escapedExists":false}.

Blocking P0/P1 findings:

  • P1 — the new explicitly named bundled fallback permits a skill name to escape the declared agent skills directory. normalizeRequested() catches normalization failure and restores the raw name; that raw value then reaches both findBundledSkillSource() and writeManagedAgentSkill() path joins. The reachable input ../../skills/todos-plan resolved the bundled source and wrote SKILL.md plus its ownership marker under the probe home's skills/todos-plan, outside .codewith/skills. The action itself reported the escaped path. With --force, the same path can overwrite an unmanaged SKILL.md outside the target agent directory. This violates the sync command's write scope and is an unsafe mutation path.

Required remedy:

  • Reject any name that does not successfully normalize before source lookup or target-path construction. Do not catch normalization errors and reuse the raw input. Add a regression proving a traversal-shaped name performs no write, while the valid named bundled fallback still writes only beneath the selected agent directory.

Non-blocking follow-ups:

  • None.

GitHub already reported this PR merged before this review completed. The finding therefore needs a forward fix on the landed code; the green gates do not waive the P1.

@andrei-hasna

Copy link
Copy Markdown
Contributor Author

The named P1 is fixed forward in commit 74cce4c23f262860a2ff3027e603477e2a8de3d3 on PR #88.

The repair rejects malformed skill names before any source lookup or target-path construction. The focused regression is now 17 pass, 0 fail; bun run typecheck exits 0; bun run test exits 0 with 1076 pass, 1 skip, 0 fail; the staged security review exits 0 with no findings.

PR #87 was already merged before the review verdict, so #88 is the forward landing path.

@andrei-hasna

Copy link
Copy Markdown
Contributor Author

The blocking path-containment finding is fixed and landed through #88.

  • Remediation head: 129b9821095a2cd88c47e6731b7e1c2ee0176791
  • Independent focused re-review: GO, no P0/P1 findings
  • CI: passed
  • Squash merge on main: 86c7058a5e65d890a4591bafe52d66207eccdc3f
  • Landed tree matches the reviewed merge tree; single Agent: Trebius trailer verified

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.

1 participant