feat(compiler): harden v1 execution contracts - #807
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: bc74f79795
ℹ️ 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".
| } | ||
| return nil, fmt.Errorf("load discovered env file %q: %w", path, err) | ||
| } | ||
| return environment.ForSubprocess(os.Environ()), nil |
There was a problem hiding this comment.
When gowdk dev is delegated to a project helper, this promotes the initial .env overlay into the helper's process environment. The long-lived helper later notices .env edits and calls envfile.Load again, but process values take precedence over file values, so every changed variable remains pinned to its startup value even though a rebuild and runtime restart occur. Preserve the original process/file distinction or restart the helper when the env file changes.
Useful? React with 👍 / 👎.
| func devRouteReloadPayloadLoaded(plan buildOptions, change inputChange) (string, bool) { | ||
| update, ok := devIncrementalSPAUpdateLoaded(plan, change) | ||
| if !ok || update.plan.LayoutChanges == 0 || update.plan.PageChanges != 0 || update.plan.ComponentChanges != 0 { | ||
| if !ok || update.plan.ComponentChanges != 0 || (update.plan.LayoutChanges == 0 && update.plan.PageChanges == 0) { |
There was a problem hiding this comment.
Allowing every page change into the route-scoped patch path breaks route edits. If a page moves from /old to /new, devRoutesForPageSources only returns the newly parsed /new route, so the browser still displaying /old rejects the update in applyDocumentPatch and neither patches nor reloads. Route-identity changes need the old route included or must use a full reload.
Useful? React with 👍 / 👎.
| }; | ||
| const syncDocumentHead = (next) => { | ||
| document.title = next.title; | ||
| const selector = 'meta[name],meta[property],link[rel="canonical"],link[rel="stylesheet"],script[data-gowdk-store]'; |
There was a problem hiding this comment.
For a page edit that changes inline or referenced JavaScript, the new document contains an updated page-scoped <script src>, but this patch selector synchronizes only store scripts and leaves all executable script tags from the old document in place. Because those modules have already executed and their stable URLs are never reinserted, the patched HTML runs stale client behavior; classify script-changing page updates as full reloads or explicitly replace and safely remount their scripts.
Useful? React with 👍 / 👎.
| if err := os.MkdirAll(filepath.Dir(absolute), 0o755); err != nil { | ||
| return "", err | ||
| } | ||
| stage, err := os.MkdirTemp(filepath.Dir(absolute), "."+filepath.Base(absolute)+".gowdk-stage-*") |
There was a problem hiding this comment.
os.MkdirTemp creates the staging directory with mode 0700, and the transaction later renames that directory directly onto the target without restoring the target's mode or applying 0755. Consequently every generated static output and app directory becomes owner-only after publication; deployments that build as one user and serve or package artifacts as another user cannot traverse the directory. Set an appropriate mode on the stage before commit, preserving an existing target mode when applicable.
Useful? React with 👍 / 👎.
| generated, err := consumer.GeneratedGo(target.target, blockContext) | ||
| if cancellable, ok := consumer.(gowdk.GoBlockConsumerContext); ok { | ||
| generated, err = cancellable.GeneratedGoContext(context.Background(), target.target, blockContext) |
There was a problem hiding this comment.
For every consumer implementing GoBlockConsumerContext, including the new executable extension proxy, this first calls GeneratedGo and then calls GeneratedGoContext for the same block. Context-aware consumers therefore execute twice, which can duplicate external side effects or make the emitted files depend on the second invocation; the matching validation path has the same double-call pattern. Select the context-aware method first and call the legacy method only in an else branch.
Useful? React with 👍 / 👎.
| for _, extension := range config.Extensions { | ||
| processors = append(processors, gowdk.ResolveExtensionCapabilities(extension).CSSProcessor) |
There was a problem hiding this comment.
This executes every resolved extension CSS processor without consulting its ExtensionDescriptor. An extension can omit ExtensionPhaseCSS and even omit the CSS capability descriptor entirely, pass ValidateExtensions, and still run build-time code during CSS planning, defeating the new explicit phase/capability contract. Validate that the descriptor declares the supported CSS phase and capability before invoking the processor.
Useful? React with 👍 / 👎.
| OmitDefaultPrefix bool | ||
| // Errors localizes stable runtime error codes. Missing entries use each | ||
| // error's safe default message. | ||
| Errors gowdki18n.ErrorBundle |
There was a problem hiding this comment.
The new I18N.Errors field is not handled by parseI18NConfig. A valid fully literal i18n.ErrorBundle passes validateConfigExpression without requesting executable evaluation, after which the AST decoder silently drops the bundle and generated handlers fall back to default-language messages. Add this field to the AST decoder or force executable loading whenever it is present.
Useful? React with 👍 / 👎.
| stmts = append(stmts, &ast.IfStmt{ | ||
| Cond: &ast.UnaryExpr{Op: token.NOT, X: call(selExpr(submission, "HasSubmitted"), stringLit(field))}, | ||
| Body: block(exprStmt(call(selExpr(id("validation"), "Add"), stringLit(field), stringLit(actionValidationMessage(action.RequiredMessages[field], "required"))))), | ||
| Body: block(exprStmt(call(selExpr(id("validation"), "AddCode"), stringLit(field), stringLit("validation_required"), stringLit(actionValidationMessage(action.RequiredMessages[field], "required")), id("nil")))), |
There was a problem hiding this comment.
Generated required-field errors pass nil for Vars, even though the documented and example validation_required catalogs use the {field} placeholder. i18n.Format leaves placeholders unchanged when no variables are supplied, so partial validation responses display literal text such as {field} is required instead of the field name in every locale. Pass at least map[string]string{"field": field} here, and the corresponding limit variables for the other generated validation codes.
Useful? React with 👍 / 👎.
1 participant
Summary
Issue Closure
Closes #771
Closes #758
Closes #726
Closes #724
Closes #722
Closes #721
Closes #720
Closes #717
Closes #716
Closes #714
Closes #697
Closes #695
Migration / Compatibility
g:forandg:ifrequireg:lane="server"org:lane="client".Config.Interopregistrations.{ok:false,error:{code,message}}.Verification
go test ./...go test ./...in every nested module listed byscripts/go-modules.shgo build ./cmd/gowdkGOTOOLCHAIN=go1.26.4 scripts/check-static-analysis.shLLM Assistance